pm/node: honor XDG_DATA_HOME for the shim dirs on a fresh install - #752
pm/node: honor XDG_DATA_HOME for the shim dirs on a fresh install#752colinhacks wants to merge 10 commits into
Conversation
nub resolves its cache root through XDG_CACHE_HOME and its config root
through XDG_CONFIG_HOME, but the two shim dirs were derived from the home
directory alone, so redirecting every XDG variable still left `nub pm shim`
writing to `~/.nub/shims` and editing the real shell profiles.
A FRESH install now uses `$XDG_DATA_HOME/nub/<leaf>` when that variable is
set. An existing `~/.nub/<leaf>` always wins: relocating a shim dir that is
already installed would leave the profile's PATH line naming the old path, so
the newly written shims would sit where nothing resolves and a bare `npm` or
`pnpm` would silently keep running the pre-upgrade binaries.
Three consumers assumed the dir was a fixed path and needed fixing with it:
- `invoked_as_persistent_node_shim` compared against the resolved dir. A
shim installed under XDG and run from a shell without the variable failed
the comparison, and the global `node` would have started AUGMENTING every
node process on the machine.
- `which_node`'s recursion guard skipped the shim dir by exact path. The
same mismatch would have let nub resolve its own shim as the real node.
Both now match on the SHAPE of the directory (a `node-shim` leaf under a
`nub`/`.nub` parent), which holds under either root.
- Removal resolved a single dir, so unshimming an XDG install from a shell
without the variable reported success while orphaning the shim binaries.
Removal now sweeps every candidate root. A custom XDG_DATA_HOME that is
unset by the time unshim runs is genuinely unknowable, so that case warns
instead of reporting a clean removal.
The block's `dir_marker` drops its leading dot so one descriptor strips a
legacy or an XDG PATH line, keeping unshim working across the boundary.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Important
The resolution rule and the in-crate consumers look right, but the enumeration of consumers is incomplete: the three installer/upgrade shim refreshers outside nub-core still look only in ~/.nub/shims, and nub node unshim did not get the sweep-aware reporting nub pm unshim got.
Reviewed changes — the full diff at 2b2b3df (4 files, 1 commit), plus the out-of-diff consumers of shim_dir() / node_shim_dir() across install.sh, install.ps1, npm/nub/postinstall.js, cli.rs and the shim test suites.
resolve_shim_dirseam — newpm::shim::resolve_shim_dir(home, xdg_data, leaf)returns$XDG_DATA_HOME/nub/<leaf>only when the legacy~/.nub/<leaf>does not exist on disk, so XDG governs a fresh install and never relocates an installed one.- Shape-based node-shim identification —
invoked_as_persistent_node_shimswaps exact canonical-path equality foris_node_shim_dir_shape(anode-shimleaf under anub/.nubparent), andwhich_node_inadds the same predicate alongside its existing exact-path guard. - Sweeping removal —
shim_dirs_for_removalreturns three candidates (legacy, current XDG, XDG spec default, deduped);remove_shimschanges fromResult<bool>toResult<Vec<PathBuf>>andrun_pm_unshimnames each dir it removed plus warns when a PATH block was stripped with nothing found. - Dual PATH blocks — new
*_BLOCK_XDGdescriptors, withdir_markerwidened from.nub/shimstonub/shimsso one descriptor'sstrip_blockremoves either line.
⚠️ The installer and postinstall shim refreshers still look only in ~/.nub/shims
Three upgrade channels re-link the PM shims after swapping the nub binary, and all three hardcode the legacy path and bail silently when it is absent. A user whose shims live under $XDG_DATA_HOME gets no refresh at all, so npm/pnpm/yarn keep executing the pre-upgrade inode with no error — the failure install.sh:207-209 itself calls "worse than any loud breakage". Their comments also cite shim_dir() as the authority for the fixed path, which this PR makes false.
Technical details
# Installer shim refreshers are not XDG-aware
## Affected sites
- `install.sh:217` and `site/public/install.sh:217` — `local shim_dir="$HOME/.nub/shims"`, then `[[ -d "$shim_dir" ]] || return 0`. The curl channel silently skips an XDG install.
- `install.ps1:136` and `site/public/install.ps1:136` — `$shimDir = "$env:USERPROFILE\.nub\shims"`, then `if (-not (Test-Path …)) { return }`. Same on the irm channel. `xdg_data_home()` (`crates/nub-core/src/pm/shim.rs:441`) has no Windows gate, so a Windows user with the variable set reaches the XDG branch.
- `npm/nub/postinstall.js:94` — `path.join(os.homedir(), ".nub", "shims")`, then a `readdirSync` whose ENOENT catch returns. Same on `npm i -g @nubjs/nub`.
- Stale comments asserting the path is fixed: `install.sh:214-215`, `install.ps1:130-132`, `npm/nub/postinstall.js:6,67,102,168`.
- Also stale for the same reason, functionally harmless: `crates/nub-core/src/pm/shim.rs:1258-1259` ("since `~/.nub/shims` is a fixed, possibly-symlinked dir").
Not affected, for the record: `relink_shims_after_selfowned` and `shim_relink_reminder`
(`crates/nub-cli/src/cli.rs:10704,10723`) call `shim_dir()`/`node_shim_dir()` and inherit XDG-awareness.
So `nub upgrade`'s self-owned channel is fine; only the three external refreshers are not.
## Required outcome
- Each of the three refreshers resolves the shim dir the same way `resolve_shim_dir` does — legacy wins if
present, else `${XDG_DATA_HOME:-$HOME/.local/share}/nub/shims` — including the lock file that sits beside it
(`$HOME/.nub/shims.lock` today, which must move with the dir or the lock protocol stops serializing against
the Rust `ShimLock`).
- The "the shim dir is `~/.nub/shims` regardless of `NUB_INSTALL_DIR`" comments are corrected in all four
installer copies.
## Open questions for the human
- `install.ps1` targets Windows, where XDG is not a platform convention. Is honoring `XDG_DATA_HOME` on Windows
intended at all? If not, gating `xdg_data_home()` to non-Windows would remove this leg entirely and shrink
the surface.⚠️ nub node unshim did not get the sweep-aware reporting nub pm unshim got
remove_node_shim now sweeps every candidate dir, but run_node_unshim still prints node_shim_dir() — the dir that resolves now — so removing ~/.local/share/nub/node-shim from a shell without XDG_DATA_HOME reports removed /home/you/.nub/node-shim, naming a path that was never touched. The orphan warning added to run_pm_unshim has no counterpart either, so the custom-XDG-unset case reports a clean removal while the node hardlink stays on disk.
Technical details
# `run_node_unshim` reports a path the sweep may not have removed
## Affected sites
- `crates/nub-cli/src/cli.rs:10224-10245` — `run_node_unshim` calls `shim::node_shim_dir()?` for its message,
then `shim::remove_node_shim()?` which returns only `(bool, Vec<PathBuf>)`. The `bool` cannot say WHICH of the
three candidates existed.
- `crates/nub-core/src/node/shim.rs:173-190` — `remove_node_shim` collapses the sweep into `existed |= …`,
discarding the removed paths.
## Required outcome
- `nub node unshim` names the directory (or directories) it actually removed, exactly as `run_pm_unshim` now does.
- The "a PATH block was stripped but no shim dir was found" warning added at `cli.rs:10118-10128` fires for the
node shim too — that case is identical for both families.
## Suggested approach
Change `remove_node_shim` to return `(Vec<PathBuf>, Vec<PathBuf>)` (removed dirs, changed profiles), mirroring
`pm::shim::remove_shims`, and lift the shared reporting into one helper so the two verbs cannot drift again.ℹ️ Four shim tests now read the ambient XDG_DATA_HOME
resolve_shim_dir picks the XDG path whenever the variable is set and the legacy dir does not yet exist, and every one of these tests starts from a freshly created empty home. None of them clears the variable, so they pass only because GitHub runners do not export it — on a developer box or container that does, they fail. This is the hermetic-test rule in AGENTS.md, and the diff introduced the dependency.
Technical details
# Shim tests depend on the ambient environment
## Affected sites
- `crates/nub-core/src/node/shim.rs:210-217` — `node_shim_dir_is_under_the_install_surface_not_the_cache`
asserts `dir.ends_with(".nub/node-shim")` against the REAL home and REAL environment. This one is in a file
the diff already edits.
- `crates/nub-cli/tests/pm_shim.rs:553,900` — assert `home.join(".nub/shims")`. The `run` helper
(`pm_shim.rs:84-98`) removes only three `npm_*` vars; no `env_clear`.
- `crates/nub-cli/tests/node_shim.rs:58,97` — assert `home.join(".nub/node-shim")`. The `nub` helper
(`node_shim.rs:40-55`) sets only `HOME` and `SHELL`.
## Required outcome
- Every test that asserts a shim path is deterministic regardless of the launching shell's `XDG_DATA_HOME`:
either clear it in the spawn helpers, or set it explicitly and assert the path the test intends.
- The `node/shim.rs` unit test asserts through `resolve_shim_dir` with explicit arguments rather than through
the environment-reading `node_shim_dir()`.
- Ideally one test covers the fresh-XDG install end to end, since that is the behavior this PR adds and no
integration test exercises it today.ℹ️ Nitpicks
site/content/docs/pm/pm-shim.mdx:10,16andsite/content/docs/node.mdx:182,184,190,194still present~/.nub/shimsand~/.nub/node-shimas the unconditional location.AGENTS.mdtreats the docs update as part of a user-facing behavior change.crates/nub-core/src/node/discovery.rs:867-872— the doc comment forwhich_node_instill says "Two recursion guards" and describes the persistent one as skipped "by CANONICAL-PATH equality"; there are three now, and the third is the shape check.crates/nub-core/src/node/discovery.rs:883—dir.canonicalize()moved out of the short-circuited&&, so it now runs for every PATH entry even whenpersistent_shimisNone. Necessary for the shape check and cheap, but worth knowing it is one extrastatper PATH entry per node resolution..github/workflows/verify-install.yml:136hardcodes$env:USERPROFILE\.nub\shims, so the reinstall-relinks check can never catch theinstall.ps1gap above.
Claude Opus | 𝕏
The three upgrade channels that re-link the PM shims after swapping the nub binary each hardcoded `~/.nub/shims` and returned silently when it was absent, so a user whose shims live under `$XDG_DATA_HOME` got no refresh at all and kept executing the pre-upgrade inode with no error. install.sh and npm/nub/postinstall.js now refresh whichever candidate dirs EXIST. Neither reimplements resolve_shim_dir's legacy-wins rule — refreshing what is already there cannot drift from where new shims go. The lockfile moves with the dir (`<dir>.lock`, matching ShimLock::acquire's `<parent>/<name>.lock`) or it stops serializing against a concurrent `nub pm shim`. xdg_data_home() is now `#[cfg(not(windows))]`. XDG is a freedesktop convention and Windows has %LOCALAPPDATA%; gating it there keeps every Windows install on `~/.nub/shims` and leaves install.ps1 correct as written. `nub node unshim` reports the dirs the sweep actually removed rather than the one that resolves now, and warns when a PATH block was stripped with no dir found — both already true of `nub pm unshim`. Tests: the shim suites read the ambient XDG_DATA_HOME once the dir honored it, so an empty HOME plus an exporting shell sent the shims to the XDG root and failed the `~/.nub/shims` assertions. The spawn helpers clear the variable and the unit test asserts through resolve_shim_dir with explicit arguments. Verified by running all three suites with XDG_DATA_HOME exported. Also corrects four comments asserting the shim dir is a fixed path.
There was a problem hiding this comment.
Important
The three installer refreshers and nub node unshim now match the resolution rule, and the function splits are behavior-preserving. But the new installer leg ships with no test, and the harness case that covers the old one can now write outside its sandbox. The four inline threads from the previous review are unaddressed and still open.
Reviewed changes — the delta since 2b2b3df (commit b09ab74), plus the out-of-diff machinery it depends on: ShimLock::acquire, tests/installer/run.sh, cache_dir / nub_data_dir, and the Windows test and CI legs.
- Installer refreshers made XDG-aware —
install.sh,site/public/install.shandnpm/nub/postinstall.jssplit into a caller plus a per-directory worker, refreshing~/.nub/shimsand${XDG_DATA_HOME:-$HOME/.local/share}/nub/shims, whichever exist, rather than reimplementingresolve_shim_dir. - Sweep-aware
nub node unshim—remove_node_shimreturns(Vec<PathBuf>, Vec<PathBuf>), so the verb names the directories the sweep actually cleared and warns when a PATH block was stripped with nothing removed. xdg_data_home()gated to non-Windows — returnsNoneundercfg(windows), pinning every Windows shim dir to%USERPROFILE%\.nub\shims, with the contract spelled out in bothinstall.ps1copies.- Test hermeticity —
pm_shim.rs'srunandnode_shim.rs'snubclearXDG_DATA_HOME, and thenode/shim.rsunit test asserts throughresolve_shim_dirwith explicit arguments instead of the real environment. - New recursion-guard test —
which_node_skips_an_xdg_shim_dir_the_caller_could_not_nameexercises the shape guard withpersistent_shim: Noneand adds a negative control for anode-shimdirectory outside a nub root.
⚠️ The new installer XDG leg is untested, and the harness case covering the old one can now escape its sandbox
tests/installer/run.sh:247-282 is the only test of refresh_pm_shims, it runs in CI (verify-install.yml:285), and it exercises the legacy directory alone — so the leg this commit adds has no coverage on the channel where a miss is silent staleness. The same case sandboxes HOME but not XDG_DATA_HOME, and ${XDG_DATA_HOME:-$HOME/.local/share} is expanded by install.sh at run time: on a box that exports the variable to an absolute path outside the sandbox with a real nub/shims dir under it, the harness now ln -fs the developer's live npm/yarn shims onto the binary it just downloaded into a temp dir that trap clean EXIT deletes. Before this commit the refresher could only ever touch $HOME/.nub/shims, which was always inside the sandbox.
Technical details
# The installer harness neither covers nor contains the new XDG refresh leg
## Affected sites
- `tests/installer/run.sh:247-282` — `reinstall re-links existing PM shims and creates no new ones`.
`export HOME=$(mksandboxdir)` at line 251 is the only environment isolation; no `XDG_DATA_HOME` handling.
Every assertion (lines 269-280) names `$HOME/.nub/shims`, so the second candidate is never observed.
- `install.sh:225` and `site/public/install.sh:225` — the loop that introduces the out-of-sandbox candidate.
- `.github/workflows/verify-install.yml:285` — runs the harness. GitHub-hosted runners do not export
`XDG_DATA_HOME`, so the gap is latent in CI rather than red.
## Required outcome
- The harness cannot write outside `$test_sandbox_dir` regardless of the invoking shell's environment.
- The XDG refresh leg is exercised: an existing `<xdg>/nub/shims` is re-linked to the new inode, the
refresh-only contract still holds there (the four un-opted-in names must not appear), and no
`<xdg>/nub/shims.lock` is left behind.
## Suggested approach
Add `export XDG_DATA_HOME=$(mksandboxdir xdg)` beside the existing `export HOME` in that case — this both
contains the run and gives the new leg a directory to act on — then duplicate the case's shim setup and its
four assertion groups against `$XDG_DATA_HOME/nub/shims`. The other harnesses in this repo already sandbox
the variable this way (`tests/daily-driver/run.sh:50`, `tests/conformance/run.sh:63`).ℹ️ Nitpicks
npm/nub/postinstall.js:6,:67and:115still describe the shim directory and its lock as unconditionally~/.nub/shims; only the log line at:181was updated to name the directory it acted on.crates/nub-cli/tests/pm_shim.rs:130-142—run_with_timeoutdid not get theenv_remove("XDG_DATA_HOME")its siblingrundid. Its one caller (empty_path_entry_with_cwd_at_the_shim_does_not_loop) asserts no shim path, so this is latent rather than broken, but the two spawn helpers in the same file now disagree about hermeticity.
Claude Opus | 𝕏
`invoked_as_persistent_node_shim` needs BOTH the exact-path comparison and the shape check. Shape alone regressed `~/.nub` symlinked to a differently-named target (`~/.nub -> ~/dotfiles/nub-config`), where the canonical parent is `nub-config` and the shape rejects it — turning the global `node` into an augmenting one, which the node-hijack contract forbids. The fish PATH line used `set -q`, which is true for a variable that is DEFINED but empty: an exported-empty XDG_DATA_HOME took the `and` branch, substituted nothing, and put `/nub/shims` on PATH. `test -n` with the quotes retained is the correct predicate — verified in fish across set, unset and empty, with the old form reproducing the bug as a control. tests/installer/run.sh sandboxed HOME but not XDG_DATA_HOME, and install.sh now expands it at run time — so on a box exporting it the harness would `ln -f` the developer's live npm/yarn shims onto the throwaway binary it had just installed and was about to delete. Pinned inside the sandbox. Adds the missing coverage for the XDG refresh leg: re-link to the new inode, the refresh-only contract, lock cleanup beside the XDG dir, and that the legacy dir is never conjured into existence. Also folds the contradictory "Returns whether it existed" line into the `Vec<PathBuf>` return it now has. Known limitation, unchanged: an XDG install writes an env-relative PATH line that is re-evaluated per shell, so a shell without XDG_DATA_HOME resolves it away from the shims. Fixing that needs the block's lines to carry the absolute resolved path.
There was a problem hiding this comment.
ℹ️ No new issues in this delta — one stale doc comment inline.
The three first-round threads this commit targets are genuinely fixed, and the new installer case is real coverage rather than theatre: install.sh rm -rfs and re-extracts bin/, so bin/nub gets a new inode while the surviving hardlinks keep the old one alive, and the -ef assertions go red without the XDG sweep. Four inline threads from the earlier rounds are still open and unreplied; the one describing behavior rather than doc text is the unshim sweep materializing an empty ~/.local/share/nub/.
Reviewed changes — the delta since b09ab74 (commit 61bcd28), plus the machinery it turns on: fish's combiner and command-substitution semantics, install.sh's reinstall path, and the current state of every open thread.
- Restored the exact-path compare in
invoked_as_persistent_node_shim— it now tries canonical equality againstnode_shim_dir()first and falls back tois_node_shim_dir_shape, so a~/.nubsymlinked to a differently-named target is recognized again without losing the XDG-unset case. - Replaced fish's
set -qwith a quotedtest -nin both XDGfish_lines, so an exported-but-emptyXDG_DATA_HOMEtakes the default branch instead of putting/nub/shimson PATH. - Rewrote the
remove_shimsdoc so its singleReturnssentence describes theVec<PathBuf>rather than the removedbool. - Contained and covered the installer harness — the pre-existing case pins
XDG_DATA_HOMEinside its sandbox, and a newreinstall re-links PM shims under XDG_DATA_HOMEcase exercises the fresh-install shape: re-link to the new inode, the four un-opted-in names stay absent, no<xdg>/nub/shims.locksurvives, and~/.nub/shimsis not conjured into existence.
Claude Opus | 𝕏
`ShimLock::acquire` does `create_dir_all(parent)` before taking the lock, so sweeping a candidate root the user never installed into MATERIALIZED it. On a clean home `nub pm unshim` plus `nub node unshim` left four empty directories behind — `~/.nub`, `~/.local`, `~/.local/share`, `~/.local/share/nub`. A command whose only job is removal must not create anything, least of all outside the directory it was asked about. Both sweeps now skip an absent candidate before reaching the lock. `~/.nub` was already created this way before the sweep existed; the two under `.local` were new, so this fixes the regression and the pre-existing papercut together. Verified end to end: a clean home now gains 0 entries where it gained 4, and a real install still unshims to completion.
There was a problem hiding this comment.
ℹ️ No new issues in this delta — the guard is correct; its test is the one thing inline.
The if !dir.exists() { continue; } added to both sweeps does what the open thread asked: remove_shims_from is reached only from these two loops (plus tests), it already mapped NotFound to Ok(false), and ShimLock serializes writers — so skipping an absent candidate before the lock changes nothing except that ShimLock::acquire's create_dir_all(parent) no longer materializes ~/.local/share/nub/. That thread is resolved. Three inline threads from the earlier rounds are still open and unreplied, all describing doc text or test strength rather than behavior.
Reviewed changes — the delta since 61bcd28 (commit 5d2d224), plus the machinery it turns on: ShimLock::acquire, every caller of remove_shims_from, and the sibling unit tests that assert the removal seam's idempotence.
- Skipped absent candidates in both removal sweeps —
remove_shimsandremove_node_shimnowcontinuepast a candidate directory that does not exist, so an unshim can no longer create the very root it was searching for. - Added a no-litter regression test —
removing_shims_never_creates_a_root_that_was_not_thereasserts the XDG root, the XDG spec default and the legacy root are all absent after a sweep over a pristine home.
Claude Opus | 𝕏
The previous commit put the same `if !dir.exists()` in both removal sweeps, each commented as pointing at the other, so dropping one changed nothing that any test noticed. And the test that was meant to prove it re-implemented the guard in its own loop — deleting the production check left it green — with a dead `if dir.exists()` branch under an assert that already guaranteed the negation. The check now lives once, in `remove_shims_from` immediately before `ShimLock::acquire`, which is the call whose `create_dir_all(parent)` does the materializing. Semantics are unchanged: that function already mapped NotFound from `remove_dir_all` to `Ok(false)`, and its only callers are the two sweeps. The test now calls `remove_shims_from` unconditionally for every candidate and asserts both the `false` return and that no root appeared. Verified by deleting the early return and watching it fail.
There was a problem hiding this comment.
ℹ️ No new issues in this delta. Not approving only because four inline threads from earlier rounds are still open.
The hoist does exactly what the open thread asked, and the test is now a genuine positive control rather than a re-implementation of the guard: with the early return deleted, ShimLock::acquire's create_dir_all(parent) materializes <home>/.nub, <home>/xdg-data/nub and <home>/.local/share/nub, and all three !root.exists() assertions go red. remove_shims_from has no callers outside the two sweeps and the unit tests, it already mapped NotFound to Ok(false), and an absent directory has no writer to serialize against — so returning before the lock changes nothing else.
Reviewed changes — the delta since 5d2d224 (commit fd093ea), plus the machinery it moves: ShimLock::acquire, every caller of remove_shims_from, and the sibling removal tests in pm/shim.rs and node/shim.rs that assert the seam's idempotence.
- Centralized the no-litter guard — the
!dir.exists()early return moved from the two sweep loops intoremove_shims_from, ahead ofShimLock::acquire, soremove_shimsandremove_node_shimcan no longer drift apart on the contract. - Made the regression test exercise the real seam —
removing_shims_never_creates_a_root_that_was_not_therenow callsremove_shims_fromunconditionally for every candidate and asserts both thefalsereturn and the absence of all three roots, replacing the caller-sideexists()check and the deadif dir.exists()branch it used to carry.
Worth recording for a future reader: Path::exists() rather than try_exists()? is the right call in a sweep over speculative candidates. It swallows a stat error and skips that candidate, where try_exists()? would abort the whole unshim because one root nobody ever installed into could not be stat'd.
Still open and unreplied, all doc text or test strength rather than behavior: the pm_shim_block "can never disagree" doc, the xdg_data_home() Windows rationale, the weakened node_shim_dir_is_under_the_install_surface_not_the_cache, and the invoked_as_persistent_node_shim paragraph that 61bcd28 made false (that one carries a ready suggestion block).
Claude Opus | 𝕏
Four things the reviews were right about, all mine. The Windows gate's rationale was factually wrong. It cited `cache_dir` as branching on Windows for the same reason, but `cache_dir` reads XDG_CACHE_HOME BEFORE its cfg(windows) split — its own doc says an explicit value "still wins everywhere" — and `nub_data_dir_from` does the same. This gate is therefore nub's one surface that refuses an explicitly-set XDG variable on Windows, and the real reason is the cross-language contract: install.ps1 re-links from a single hardcoded path and cannot share resolve_shim_dir, so honoring the variable there would strand shims the irm channel never refreshes. `invoked_as_persistent_node_shim`'s doc still said the match was by shape "rather than by comparing against node_shim_dir", which 61bcd28 made false when it restored the exact compare — the check that covers a symlinked ~/.nub. `node_shim_dir_is_under_the_install_surface_not_the_cache` lost its subject when it was made hermetic: it stopped calling node_shim_dir(), so repointing that at the cache root would still have passed, and its second assertion could not fail. It now compares node_shim_dir() against the real cache_dir(), which no XDG variable can make contain the other — hermetic without clearing anything, verified with both XDG_DATA_HOME and XDG_CACHE_HOME exported. `pm_shim_block`'s "can never disagree" holds at install time only. The dir is fixed on disk while the PATH line is re-expanded per shell, and a nonexistent entry is skipped silently. Documented as XDG's contract rather than a nub defect, with the cost of pinning it recorded.
There was a problem hiding this comment.
ℹ️ No blocking issues in this delta — all four open threads are genuinely closed and now resolved. One overclaiming comment inline, plus a user-facing string this doc-correction pass missed.
Reviewed changes — the delta since fd093ea (commit dd7177d), plus the sources its new claims rest on: node::discovery::cache_dir, pm_engine::nub_data_dir_from, and the restored body of invoked_as_persistent_node_shim.
- Corrected the
xdg_data_home()Windows rationale — the gate is now justified by theinstall.ps1cross-language contract instead of acache_dirprecedent that said the opposite. Both new neighbour claims check out:discovery.rs:986readsXDG_CACHE_HOMEbefore thecfg(windows)split at:990, andpm_engine/mod.rs:2825readsXDG_DATA_HOMEbefore itswindowsbranch at:2828. - Rewrote the
invoked_as_persistent_node_shimdoc — it now states that both the exactnode_shim_dircompare and the shape fallback are load-bearing, and names the case each one covers, matching the body restored in61bcd28. - Scoped
pm_shim_block's guarantee to install time — the doc records that${XDG_DATA_HOME:-$HOME/.local/share}is re-expanded at every shell startup, so the written line can silently stop naming the installed directory, and what pinning it would cost. - Restored the subject of
node_shim_dir_is_under_the_install_surface_not_the_cache— it calls the realnode_shim_dir()again and asserts it is not under the realcache_dir(). Repointing the shim dir at the cache root now makesdir.starts_with(&cache)fire, which the previousresolve_shim_dir("/home/probe", …)form could not.
ℹ️ nub pm --help still tells the user the shims go in ~/.nub/shims
This commit swept the shim-dir docs but not the one the user actually sees: cli.rs:9160 prints shim link npm/pnpm/yarn shims into ~/.nub/shims, which is wrong for exactly the fresh XDG installs this PR adds. Emitted output outranks a doc comment here, and the docs pages flagged in the first round are still in the same state.
Technical details
# User-facing surfaces still present the shim dir as unconditional
## Affected sites
- `crates/nub-cli/src/cli.rs:9160` — the `nub pm --help` body: `shim link npm/pnpm/yarn shims
into ~/.nub/shims (re-run after `nub upgrade`)`. Printed output, not a doc comment.
- `site/content/docs/pm/pm-shim.mdx:10,16` and `site/content/docs/node.mdx:182,184,190,194` — still
present `~/.nub/shims` / `~/.nub/node-shim` as the location. `AGENTS.md` treats the
`site/content/docs/` update as part of a user-facing behavior change.
## Required outcome
- The help string and the two docs pages describe the resolution rule the code implements: legacy
`~/.nub/<leaf>` when it already exists, `$XDG_DATA_HOME/nub/<leaf>` on a fresh install with the
variable set, and unix-only for the XDG half.ℹ️ Nitpicks
crates/nub-core/src/node/shim.rs:5(module doc) andcrates/nub-core/src/pm/shim.rs:7still name the dedicated dir as~/.nub/node-shim/~/.nub/shimswith no XDG qualifier — same forcli.rs:10132and:10220. Internal only, unlike the help string above.remove_shims_from's doc (pm/shim.rs:707) still reads "[remove_shims] with an explicit dir (the testable body)", which stopped being literally true atb09ab74whenremove_shimsbecame a sweep over three candidates.
Claude Opus | 𝕏
`nub pm --help` printed "link npm/pnpm/yarn shims into ~/.nub/shims", which is wrong for exactly the fresh XDG installs this branch adds. Emitted output outranks the doc comments the previous commit spent its time on, and site/content/docs was never touched at all — AGENTS.md treats that as part of a user-facing behavior change, not a follow-up. The help line no longer names a fixed path, and pm-shim.mdx and node.mdx state the rule: `~/.nub/<leaf>` normally, `$XDG_DATA_HOME/nub/<leaf>` on a fresh unix install, an existing dir always wins. Also softens a claim that was not true. Comparing node_shim_dir() against the real cache_dir() is NOT independent of where the roots point: with XDG_DATA_HOME and XDG_CACHE_HOME on the same root — a shape sandboxed CI images use — cache_dir() is `/x/nub` and the shim dir is `/x/nub/node-shim`, so the containment assertion fires. That layout is the user's, not nub's, so the check skips it rather than failing for something the test does not govern.
There was a problem hiding this comment.
ℹ️ No blocking issues in this delta — the user-facing gap is closed and every claim checks out. One over-broad test guard inline.
Reviewed changes — the delta since dd7177d (commit e8a72b0), plus the surfaces it claims to have swept: every ~/.nub/shims / ~/.nub/node-shim string under crates/, site/ and README.md, resolve_shim_dir, cache_dir, and the CI workflows' XDG environment.
- Dropped the fixed path from
nub pm --help— the line now readslink npm/pnpm/yarn shims onto PATH, which is true under either root. The string has no other occurrence in the repo, so no test asserted the old wording. - Documented the resolution rule on both docs pages —
pm-shim.mdx:10andnode.mdx:190now state legacy-wins plus the unix-only XDG fresh-install case, matchingresolve_shim_direxactly. The remaining~/.nub/…strings on those pages are console-output examples and are correct as the default case. - Corrected two internal doc comments — the
node::shimmodule header names the XDG alternative and points atresolve_shim_dir;remove_shims_fromno longer calls itself "remove_shimswith an explicit dir", which stopped being true once removal became a sweep. - Softened the cache-containment test's hermeticity claim — the comment now names the collapsed-roots layout instead of asserting it cannot happen, and the containment check is skipped there rather than failing.
xdg_data_homeispub(crate)andcache_dirispub, so both references resolve from the test module.
Claude Opus | 𝕏
The skip compared the two XDG ROOTS, which is the wrong question in both directions. `cache.starts_with(&data)` fires whenever the data root is merely an ancestor of the cache — `XDG_DATA_HOME=$HOME` with the default `~/.cache/nub`, where the shim is `$HOME/nub/node-shim` and nowhere near the cache — and without a data-root test a LEGACY `~/.nub/node-shim` skipped an assertion that would have passed. Adding `dir.starts_with(&data)` alone (the reviewed suggestion) narrows it but still skips that ancestor case, since both conjuncts hold there. What the test actually cares about is whether the shim LANDED in the cache, so the predicate is now `dir.starts_with(&data) && dir.starts_with(&cache)`. Verified across all three layouts: clean env asserts and passes, collapsed roots (XDG_DATA_HOME = XDG_CACHE_HOME) skip and pass, data-root-only asserts and passes.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the delta since e8a72b0 (commit 1420c8c), a single hunk in the node/shim.rs cache-containment test, plus the predicate's behavior against cache_dir, resolve_shim_dir and the legacy layout.
- Narrowed the cache-containment skip to the layout it excuses —
user_collapsed_the_rootsis nowdir.starts_with(&data) && dir.starts_with(&cache), replacing the root-vs-root comparison.
The new form is tighter than the suggestion it answers. The second conjunct is literally the assertion's failure condition, so the skip can only ever suppress a failure that would genuinely fire, and the first conjunct confines it to an XDG-derived directory. Both false-skip directions from the previous form are closed: a data root merely ancestral to the cache (XDG_DATA_HOME=$HOME with the default ~/.cache/nub) no longer matches, and a legacy ~/.nub/node-shim cannot be skipped, since cache_dir always appends nub and no ordinary XDG_CACHE_HOME makes ~/.nub/node-shim a descendant of <XDG_CACHE_HOME>/nub. The comment above it names both directions accurately.
That was the last open thread on this PR; it is replied to and resolved.
Claude Opus | 𝕏
The shim dir now resolves ONE way, in the shape every comparable tool uses —
explicit XDG first, platform default second:
1. $XDG_DATA_HOME/nub/<leaf> (any platform)
2. %LOCALAPPDATA%\nub\<leaf> (Windows)
3. ~/.local/share/nub/<leaf>
The legacy-wins branch is gone. Keeping an existing `~/.nub/<leaf>` bought
migration-freedom and cost a permanent second location every consumer had to
know about: two PATH-block variants per family, a block-selector function, a
widened strip marker, multi-candidate removal, and a dual sweep in three
installers. nub takes breaking changes between minor versions, so the dir moves
once and `nub pm shim` / `nub node shim` MIGRATE the old one — removing it and
its PATH block before writing the new one, and saying so — rather than the
codebase carrying both forever.
xdg_data_home() is no longer #[cfg(not(windows))]. Refusing an explicitly-set
XDG variable on Windows made this nub's only surface that did: cache_dir reads
XDG_CACHE_HOME above its cfg(windows) split, nub_data_dir_from does the same,
and so do pnpm's getDataDir and corepack. install.ps1 learned the same rule, so
the gate is no longer load-bearing.
macOS resolves to ~/.local/share, not ~/Library: nub already uses ~/.cache/nub
and ~/.config/nub on every unix, as do uv and mise.
Two breaks this move caused, both found by the existing suite and both from
code that assumed a RELATIONSHIP between paths rather than a value:
- nub_passthrough_target derived the official binary as the shim dir's
sibling (`<install>/shims` -> `<install>/bin/nub`). After the move that
named a path that does not exist, so the shim-dir `nub` ran its own stale
bytes instead of deferring. It now derives from the install root.
- the same function's "am I in the shim dir?" guard compared only against the
RESOLVED dir, so a user mid-migration — still holding ~/.nub/shims/nub on
PATH — stopped deferring. It recognizes the pre-move dir too.
Installers, docs, the pm help line and the tests all follow the one rule; the
pre-move path stays in each installer's sweep so an install predating the move
keeps being refreshed until its next `nub pm shim`.
There was a problem hiding this comment.
Important
The one-rule collapse is the right call and the migration path is sound. But ungating XDG_DATA_HOME on Windows — which this commit does deliberately — opened a fourth root, %LOCALAPPDATA%\nub\<leaf>, that neither the removal sweep nor the passthrough guard can reach. install.ps1 already sweeps both Windows roots; the Rust side and postinstall.js pick one.
Reviewed changes — the delta since 1420c8c (commit 5287636), plus the machinery the reversal turns on: every consumer of resolve_shim_dir, ShimLock's sibling-lock protocol across all four language copies, nub_passthrough_target's re-exec decision, and which candidate roots tests/installer/run.sh actually exercises.
- One unconditional resolution rule —
resolve_shim_diris now$XDG_DATA_HOME/nub/<leaf>→%LOCALAPPDATA%\nub\<leaf>on Windows →~/.local/share/nub/<leaf>, with no "keep an existing~/.nub/<leaf>" branch. That deletes the dual PATH-block descriptors, the widened strip marker andpm_shim_block(home)/node_shim_block(home)— a large simplification, andgrepfinds no dangling reference to any removed symbol undercrates/. ~/.nub/<leaf>demoted to pre-move —migrate_legacy_shim_dirclears it on the install path, andrun_pm_shim_install/run_node_shim_installboth strip the stale PATH block before re-adding it, so a migrating user is not left blockless.xdg_data_home()ungated on Windows — the#[cfg(windows)] -> Nonegate added two commits ago is gone, and the rationale cites pnpm'sgetDataDirand corepack reading their XDG variable above the platform branch. The precedent checks out.- Installer copies stay in lockstep —
install.shandsite/public/install.share byte-identical, as are the twoinstall.ps1copies (verified bydiff, both clean). All four writers compute the lock as<dir>.lock, matchingShimLock::acquire, so the cross-language protocol still serializes under the relocated roots.
The new installer case is real coverage rather than theatre: install.sh replaces bin/nub with a fresh inode, so the -ef assertions in reinstall re-links PM shims at the live XDG location go red without the XDG leg, and it checks the lock cleanup and the no-conjure contract on top. The nub_from_a_shim_dir_defers_to_the_official_binary rework likewise discriminates for the pre-move path — the old sibling-derived target yielded <xdg>/nub/bin/nub, so it fails without nub_install_bin_dir().
ℹ️ Doc comments in cli.rs still name the old fixed path
This commit swept the shim-dir prose in pm/shim.rs, node/shim.rs and both .mdx pages, but five doc comments in cli.rs still present ~/.nub/… as the unconditional location, and none sits inside a diff hunk so they cannot be anchored inline:
crates/nub-cli/src/cli.rs:579— thePmShimvariant doc, "via a~/.nub/shimshardlink".crates/nub-cli/src/cli.rs:3276— "the PERSISTENT globalnodeshim (nub node shim, ~/.nub/node-shim)".crates/nub-cli/src/cli.rs:9980—run_pm_shim_install's doc, "hardlink the running nub under the six PM names in~/.nub/shims".crates/nub-cli/src/cli.rs:10150—run_node_shim_install's doc, "asnodein~/.nub/node-shim".crates/nub-cli/src/cli.rs:10252—run_node_unshim's doc, "delete~/.nub/node-shim".
Internal only — the bodies all call shim_dir()/node_shim_dir() and behave correctly.
ℹ️ The PR body describes the design this commit reversed
The body still explains the superseded rule, that an existing ~/.nub/<leaf> always wins so XDG governs only a fresh install. The change is now a one-way relocation with a migration step, which is a breaking change for every user who already ran nub pm shim — their shims move and the old PATH line is rewritten. Worth saying so in the body and the title, since "honor XDG_DATA_HOME for the shim dirs on a fresh install" no longer describes what lands.
ℹ️ Coverage the reversal leaves open
- No test drives
nub_passthrough_targetfrom the live XDG shim dir, which is the casenub_install_bin_dir()was introduced for. The one test covers the pre-move dir. - Both
tests/installer/run.shcases exportXDG_DATA_HOME— the first pins it to$HOME/.local/sharefor containment, the second to$HOME/xdg-data. Nothing exercises the variable being unset, which is the branch most fresh unix installs will take. - The Windows leg of
verify-install.ymlseeds and asserts only%USERPROFILE%\.nub\shims, so the Windows resolution this commit added — both the%LOCALAPPDATA%default and the newly-ungated XDG branch — has no CI coverage at all. The PowerShell logic reads correctly, but that is inspection, not a passing test.
Claude Opus | 𝕏
| for candidate in [ | ||
| // The PRE-MOVE location. An unshim has to clean up an install made before | ||
| // the shim dir moved, or those binaries stay on disk forever with their | ||
| // PATH line stripped. | ||
| legacy_shim_dir(home, leaf), | ||
| // The XDG default, for a custom XDG_DATA_HOME that was set at install and | ||
| // is unset now — the resolution above cannot name that path. | ||
| home.join(".local").join("share").join("nub").join(leaf), | ||
| ] { |
There was a problem hiding this comment.
%LOCALAPPDATA%\nub\<leaf> is not a candidate here, and after this commit it is a root the tool genuinely installs into.
The sweep reaches it only through resolve_shim_dir at line 418, which returns it only when xdg_data is None. So on Windows with XDG_DATA_HOME set at unshim time, the three candidates are <xdg>/nub/shims, ~/.nub/shims and ~/.local/share/nub/shims — and shims installed by any earlier run, or from any shell that did not export the variable, sit untouched at %LOCALAPPDATA%.
This is the exact asymmetry the doc above already argues against. Line 426 pushes the XDG default unconditionally, precisely so a custom XDG_DATA_HOME cannot hide an install made without it; the Windows platform default needs the same treatment, and unlike the custom-XDG case the doc names as unknowable, this path is fully knowable — the process can read %LOCALAPPDATA% right now.
Two things make it worse than a cosmetic leak. run_pm_unshim's "a PATH block was removed but no shims directory was found" warning (cli.rs:10139) cannot fire on Windows: run_pm_shim_install returns at cli.rs:10061 before add_path_block(), so changed is always empty there. The user gets "<xdg>/nub/shims was already gone" and a clean exit while six shim hardlinks remain on PATH. And install.ps1:145-148 already sweeps both Windows roots (if/if, not if/elseif) with a comment explaining why — so the installer and the uninstaller now disagree about how many places shims can live.
The suggestion keeps a cfg!(windows) guard so nothing changes on unix, where LOCALAPPDATA is only ever set under Wine or a stray export.
| for candidate in [ | |
| // The PRE-MOVE location. An unshim has to clean up an install made before | |
| // the shim dir moved, or those binaries stay on disk forever with their | |
| // PATH line stripped. | |
| legacy_shim_dir(home, leaf), | |
| // The XDG default, for a custom XDG_DATA_HOME that was set at install and | |
| // is unset now — the resolution above cannot name that path. | |
| home.join(".local").join("share").join("nub").join(leaf), | |
| ] { | |
| for candidate in [ | |
| // The PRE-MOVE location. An unshim has to clean up an install made before | |
| // the shim dir moved, or those binaries stay on disk forever with their | |
| // PATH line stripped. | |
| Some(legacy_shim_dir(home, leaf)), | |
| // The XDG default, for a custom XDG_DATA_HOME that was set at install and | |
| // is unset now — the resolution above cannot name that path. | |
| Some(home.join(".local").join("share").join("nub").join(leaf)), | |
| // The Windows default, for the mirror case: an install made without | |
| // XDG_DATA_HOME, unshimmed from a shell that exports it. Unlike the | |
| // custom-XDG case above this path IS knowable, and install.ps1 already | |
| // refreshes both Windows roots. | |
| local_app_data | |
| .filter(|_| cfg!(windows)) | |
| .map(|l| l.join("nub").join(leaf)), | |
| ] | |
| .into_iter() | |
| .flatten() | |
| { |
| let home = dirs_next::home_dir(); | ||
| let is_a_shim_dir = [ | ||
| shim_dir().ok(), | ||
| home.as_ref().map(|h| legacy_shim_dir(h, SHIMS_LEAF)), |
There was a problem hiding this comment.
Same gap as the removal sweep, one layer more damaging: is_a_shim_dir is built from shim_dir() — re-resolved now — and the pre-move dir, with no %LOCALAPPDATA% candidate.
So a Windows shim installed at %LOCALAPPDATA%\nub\shims and later invoked from a shell that exports XDG_DATA_HOME matches neither entry. is_a_shim_dir is false, the function returns None, and the shim runs its own stale bytes rather than deferring to ~/.nub/bin/nub. That is precisely the failure the doc at lines 1400-1411 describes — nub upgrade reports success while nub -v shows the old version — reintroduced through a different door.
Adding local_app_data().filter(|_| cfg!(windows)).map(|l| l.join("nub").join(SHIMS_LEAF)) to the array closes it, and pairs with the fix on shim_dirs_for_removal above. Worth noting the general shape: any function that has to answer "is this one of our shim dirs?" needs the full candidate set rather than the live resolution — that is now two of them, so a shared shim_dir_candidates() helper would keep the two from drifting again.
| if (process.env.XDG_DATA_HOME) { | ||
| roots.push(process.env.XDG_DATA_HOME); | ||
| } else if (process.platform === "win32" && process.env.LOCALAPPDATA) { | ||
| roots.push(process.env.LOCALAPPDATA); | ||
| } else { | ||
| roots.push(path.join(home, ".local", "share")); | ||
| } |
There was a problem hiding this comment.
This else if makes the npm channel the odd one out. install.ps1:145-148 builds its root list with two independent ifs, so a Windows box with XDG_DATA_HOME set refreshes <xdg>\nub\shims and %LOCALAPPDATA%\nub\shims; here the else if means setting the variable silently drops the %LOCALAPPDATA% root from the sweep.
The consequence is the one the comment block directly above calls out by name: shims left pointing at the pre-upgrade inode, executing old bytes with no error. refreshShimsIn already skips an absent directory, so pushing both roots costs nothing when only one exists.
const roots = [];
if (process.env.XDG_DATA_HOME) {
roots.push(process.env.XDG_DATA_HOME);
}
if (process.platform === "win32" && process.env.LOCALAPPDATA) {
roots.push(process.env.LOCALAPPDATA);
}
if (roots.length === 0) {
roots.push(path.join(home, ".local", "share"));
}Mirroring install.ps1 is the cheaper direction than teaching install.ps1 to narrow, since the refresher is best-effort and an extra skipped directory is free.
| The shimmed `node` runs stock Node, unchanged. It resolves and provisions the version — the [precedence above](#version-precedence) — but adds nothing else: no TypeScript, no injected globals, no automatic `.env`. So `node app.ts` type-strips exactly as stock Node does, while `nub app.ts` transpiles and runs it. | ||
|
|
||
| Like [`nub pm shim`](/docs/pm/pm-shim), it is opt-in and reversible. It installs a `node` link in `~/.nub/node-shim` and adds that directory to your PATH; because that comes first, a Node you install later (`brew install node`) is shadowed until you unshim. Re-run `nub node shim` after `nub upgrade` to re-link. | ||
| Like [`nub pm shim`](/docs/pm/pm-shim), it is opt-in and reversible. It installs a `node` link in `$XDG_DATA_HOME/nub/node-shim` — `~/.local/share/nub/node-shim` when that variable is unset — and adds that directory to your PATH; because that comes first, a Node you install later (`brew install node`) is shadowed until you unshim. Re-run `nub node shim` after `nub upgrade` to re-link. |
There was a problem hiding this comment.
This states two of the three branches — $XDG_DATA_HOME/nub/node-shim and the ~/.local/share default — but omits %LOCALAPPDATA%\nub\node-shim, which resolve_shim_dir returns on Windows and which nub node shim reaches through the same call.
pm-shim.mdx:10 in this same commit states all three, so the two pages now describe the same rule differently. Matching that sentence's phrasing would settle it: "$XDG_DATA_HOME/nub/node-shim when that variable is set, %LOCALAPPDATA%\nub\node-shim on Windows, and ~/.local/share/nub/node-shim otherwise".
# Conflicts: # crates/nub-core/src/node/discovery.rs

A fresh install now uses
$XDG_DATA_HOME/nub/<leaf>when that variable is set. An existing~/.nub/<leaf>always wins: relocating an installed shim dir would leave the profile's PATH line naming the old path, so a barenpmwould silently keep running pre-upgrade binaries.Three consumers assumed a fixed path and are fixed with it:
invoked_as_persistent_node_shim— a mismatch would have made the globalnodeAUGMENT every node process.which_node's recursion guard — a mismatch would have let nub resolve its own shim as the real node.Closes #750