fix(pm): make a global install produce a runnable command - #773
fix(pm): make a global install produce a runnable command#773colinhacks wants to merge 23 commits into
Conversation
`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/<alias>` 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.
`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/<alias>` 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.
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 `<home>/bin`, one level below what this returned. Split the two roots. Bins resolve to the shared user-binary directory the system already exposes: `<PREFIX>_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 `<data>/<ns>/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.
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.
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.
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.
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.
`-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 #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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
# Conflicts: # wiki/agents.md
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
On Windows a global install still doesn't produce a runnable command — and it now writes a bash export line into a .profile no Windows shell reads. Separately, the new ownership guard can't see the .cmd shim that is the thing worth protecting on that platform. Both are inline below.
Reviewed changes
Read the full diff end-to-end across all 16 files: the global layout and bin-ownership rework in vendor/aube, the host-side PATH wiring, the Cow-ified shim/profile plumbing in nub-core, and the installer/docs changes.
- Both global roots relocate. Bins go to
XDG_BIN_HOME→~/.local/bin(the uv/pipx convention) instead ofPNPM_HOME/$XDG_DATA_HOME/pnpm/~/Library/pnpm; packages go to<PREFIX>_HOME→$XDG_DATA_HOME/<ns>→~/.local/share/<ns>instead of<home>/global-nub.platform_default()and its pnpm-named path table are deleted outright — good, that was a brand-boundary violation. - Ownership guard on link.
bin_slot_is_writablerefuses to clobber an occupied slot nub didn't create;link_binsskips with a warning rather than failing the whole install. The regular-file branch keys off the# aube-bin-shim v1 target=marker, which no other PM emits — it cannot mistakenly claim an npm/pnpm/yarn shim. - Remove/update stop leaking bins.
bin_names_for→owned_binscarrying a resolvedtarget, so a link resolving throughnode_modules/<alias>into the shared store is now recognized instead of left dangling;remove_packagetakes akeep_binsset so a partial removal doesn't strip a survivor's bins. - PATH wiring after
add -g, through the marker-keyed block path, plus a newProfileOutcome::Rewrittenthat updates a stale line in place instead of appending a second block.install.shgains agrep -qxFcheck — verified exact-match against the verbatim line it writes, so repeat runs are genuinely idempotent now. - Tests.
tests/global-install/run.shis a real 143-line e2e harness that documents the five vacuous-pass traps hit while writing it; the two newshim.rsunit tests assert falsifiable properties (the stale directory must be gone, not merely outranked). I tracedinstall -g→install_to_add_args→add -g→run_addand confirmed the wiring does cover the form the PR body names.
⚠️ Nothing migrates existing global installs off the old layout
Both roots move in this PR, and I could not find a migration, a read-side fallback, or a warning anywhere in the tree. For anyone with packages already installed globally under the old layout, after upgrading:
| Surface | Behavior after upgrade |
|---|---|
nub list -g |
Old packages are invisible — it scans the new package root only. |
nub remove -g <pkg> |
Cannot find the package; the old bins stay linked forever. |
nub update -g |
Same — the old install is unreachable, so it is never updated again. |
| Old bin dir | Orphaned symlinks remain on PATH, potentially shadowing the new ones depending on PATH order. |
The new docs section in site/content/docs/install/index.mdx doesn't mention the move either, so the first signal a user gets is a command that stopped being managed.
Worth deciding explicitly rather than by omission — the options are roughly: one-shot migration on first global operation; a read-side fallback that scans the old root and warns; or "accept the break" plus a release note and a docs line telling people to re-install. It's a call about product behavior, so it's yours rather than something I'd just assert.
Related loose end regardless of which way you go: the settings docs still describe the deleted fallback chain. vendor/aube/crates/aube-settings/settings.toml:2800,2810 still says the global dir falls back to AUBE_HOME / PNPM_HOME / a platform default, and that text is mirrored into generated vendor/aube/docs/settings/index.md:2662,2673.
ℹ️ Nitpicks
crates/nub-cli/src/cli.rs— the newRewrittenarms carry "Not reachable for these two families — their directories are compile-time constants, so the line beneath the marker never differs." It is reachable: a user who hand-edits the line under# nub shimsnow gets it silently rewritten rather than duplicated. That's arguably the better behavior, but the comment says the arm is dead code when it isn't.nub update -gdoesn't callwire_global_bin_path, whileadd -gdoes. Probably fine (an update implies a prior add), but it means a user who installed globally before this change and then updates gets no wiring.tests/global-install/run.shdoesn't appear to be referenced by any workflow, so the harness only runs when someone remembers to. Given it needs network, a manual/nightly job rather than PR-gating seems right — but as-is nothing runs it.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new code to review — the head moved by a merge from
mainonly.
Reviewed changes
Range-diffed 1952c6f..627ec2d. The one new commit is a merge of origin/main; nothing in the PR's own scope changed.
- Inherited from
main, not this PR —.github/workflows/release.ymlandcrates/nub-cli/src/compile/native_layout.rscome across from #772 and correctly do not appear in the PR diff. - The
wiki/agents.mdhunk dropped out — an equivalent correction to the aube-parity guidance landed onmainindependently, so the branch no longer needs to carry it. The PR is 15 files now rather than 16.
Everything in my previous review still stands. crates/nub-cli/src/pm_engine/install_family.rs and vendor/aube/crates/aube/src/commands/global.rs are byte-identical across this range, so all three inline threads remain correctly anchored.
Claude Opus ｜ 𝕏
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 `<name>.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.
There was a problem hiding this comment.
Caution
The Windows ownership guard now rejects nub's own shims. Two of the three files create_bin_shim writes carry nothing either parser can read, so a second add -g of an already-installed package refuses to relink — and the teardown that follows deletes the working command.
Reviewed changes
Range-diffed 627ec2d..e46c57b. The single new commit closes all three inline findings from the previous review; the fix to the third one is where the new defect lives.
- Guarded the Windows PATH wiring in
wire_global_bin_pathso it prints the bin dir instead of silently writingexport PATH=…into a.profileno Windows shell reads. - Drove the ownership guard off the writer's own path list —
win_shim_pathsis nowpub, so the guard can no longer drift from the three files the writer emits. - Fixed the dangling-link misclassification by comparing against both the lexical and the canonical form of the package dir (
pkg_lex+pkg_canon). - Tightened the regular-file branch: ownership is now decided by where the embedded target resolves, not by the mere presence of a shim shape — npm, pnpm and yarn all emit wrappers of the same shape.
- Added
win_shim_target_is_recovered_from_both_wrapper_shapes, which runs on every platform.
⚠️ The assertion that would have caught this is #[cfg(unix)]-gated
bin_slot_is_writable_only_when_the_occupant_is_ours (vendor/aube/crates/aube/src/commands/global.rs:927-977) already asserts exactly the round-trip that fails — its last case is "our own prior link is ours to replace on a re-add" — but the #[cfg(unix)] attribute compiles it away on the windows-latest leg of .github/workflows/aube-parity.yml, so that leg reports green on the platform where the round-trip is broken. The one new test exercises parse_win_shim_target against hand-written strings only; nothing composes the parser with slot_entry_is_ours over the files create_bin_shim actually wrote.
Technical details
The unix gate is there because the fixtures use std::os::unix::fs::symlink. The regular-file cases don't need a symlink: writing the three shim bodies with fs::write and asserting bin_slot_is_writable returns true would run everywhere and would have caught this. A cross-platform test that writes the exact bytes generate_ps1_shim and generate_sh_shim produce is the durable guard — the failure mode here is precisely that the guard and the writers disagree, and only a test that consumes the writers' output can see that.
ℹ️ Nitpicks
- The skip warning at
global.rs:482always namesbin_dir.join(&name), but on Windows the occupant that actually triggered the skip is usually<name>.cmdor<name>.ps1. The user is pointed at a path that may not exist. global.rs:399-402's comment still describes the old marker-based rule ("carries the markercreate_bin_shimwrites") even though the code below it now resolves the target instead. The replacement comment landed a few lines down; this one is a leftover.
Claude Opus ｜ 𝕏
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 `<name>.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.
There was a problem hiding this comment.
ℹ️ The Windows self-lockout is fixed correctly. The test that would prove it stays compiled out on Windows.
Reviewed changes
Range-diffed e46c57b..40b74cf. One new commit, one substantive hunk: the #[cfg(windows)] arm of bin_slot_is_writable now decides the slot from {name}.cmd alone and treats the .ps1 / extensionless siblings as occupied-only. That is the fix the prior thread asked for, and I verified it against the writer rather than the comment:
create_bin_shim's#[cfg(windows)]branch (aube-linker/src/sys.rs:243-295) writes all three files unconditionally, so a bin this tool owns always has a.cmd— keying on it can't miss our own shim.prefer_symlinked_executablesis POSIX-only, so there is no Windows path that emits a symlink without the.cmd.- The three cases now resolve as:
.cmdpresent → delegate toslot_entry_is_ours;.cmdabsent with a sibling present →false(conservative skip); slot empty →true. Re-adding an installed package no longer skips every bin, so the empty-linked_setteardown inadd/global.rs:330-337stays armed. - Also checked the one way the parser could go wrong:
shim_node_path's cmd dialect emits@SET NODE_PATH=%~dp0…unquoted, andparse_win_shim_target's second branch requires a closing", so that line falls through instead of being recovered as the target.
prefix.rs, remove.rs, update.rs and global_install.bats carry only the rename fallout from the earlier commits and are unchanged in this delta.
Claude Opus ｜ 𝕏
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.
There was a problem hiding this comment.
ℹ️ The parser is now bound to the writer, as asked. The second reader of that format didn't come along, and the negative case lost its only assertion.
Reviewed changes
Range-diffed 40b74cf..ea5775b. One new commit, three files, all of it about where the Windows .cmd parser lives and what it is checked against.
- Moved
parse_win_shim_targetintoaube-linker. Out ofcommands/global.rs, intosys.rsnext to the writer, re-exported fromlib.rs, and ungated so it compiles on every platform.slot_entry_is_oursnow callsaube_linker::parse_win_shim_target. - Relaxed
generate_cmd_shimto#[cfg(any(windows, test))]so a non-Windows test build can drive the writer. Its one production call site (sys.rs:293) sits inside the#[cfg(windows)]arm ofcreate_bin_shim, so nothing references it in a non-Windows release build and the gate is sound. - Replaced the fixture test with two writer round-trips.
parse_win_shim_target_recovers_what_generate_cmd_shim_embedscovers bothBinLauncharms, andparse_win_shim_target_ignores_the_node_path_linepins the@SET NODE_PATH=%~dp0…line that carries no closing quote. Both run on the ubuntu and windows legs ofaube-parity.yml, which iscargo test --workspaceinsidevendor/aube.
I also chased the one way this could have been serious — whether the parser can return Some(...) for a .cmd some other tool wrote, which is how nub would overwrite a foreign command. npm is structurally immune: its cmd-shim sets SET dp0=%~dp0 once and builds every path from %dp0%, so the literal %~dp0\ the second branch requires never appears. pnpm and yarn classic both use @zkochan/cmd-shim, which emits "%~dp0\<target>" — byte-identical in shape to what generate_cmd_shim produces — so the parser does recover a target from their shims. What stops the overwrite is not the parser but the containment test at the end of slot_entry_is_ours: their target resolves into their own tree, never under nub's pkg_dir. Correct outcome, though it's worth knowing the parser is not the thing telling them apart.
ℹ️ Nitpicks
- The new test block isn't rustfmt-clean.
rustfmt --edition 2024 --check vendor/aube/crates/aube-linker/src/sys.rsreports three hunks, all introduced here: thefor launch in [...]array wants to break across lines, thegenerate_cmd_shim(...)call atsys.rs:2893is 122 columns, and there's a trailing blank line before the closing}ofmod tests. Nothing will catch it —vendor/aubeisexcluded from the root workspace so rootcargo fmt --checknever reaches it, andaube-parity.ymlruns onlycargo test. It surfaces later as unrelated noise in agit subtree splitor an upstream sync.
Claude Opus ｜ 𝕏
`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\<target>"` shape this crate does, so their wrappers parse here too. Only where the recovered target resolves decides ownership.
There was a problem hiding this comment.
ℹ️ Both threads from the last review are closed correctly. One claim in the new test's doc comment doesn't hold.
Reviewed changes
Range-diffed ea5775b..1cf6a0e. One commit, three files, all of it follow-through on the two open threads.
- Collapsed the second parser.
unlink_bins's#[cfg(windows)]arm (commands/global.rs:613) now callsaube_linker::parse_win_shim_targetinstead of its inlined copy. The swap is behaviour-preserving: the removed closure and the shared function are the same two branches token for token, andfilter_map(f).next()isfind_map(f). The only%~dp0left inglobal.rsis the comment at line 405, so there is genuinely one implementation now, and the delete path is covered by the round-trip test. - Rewrote the parser's doc comment (
sys.rs:815-831) — it now records the function as the single reader of the format and carries the pnpm/yarn caveat, so a later reader won't mistake the parse for the ownership test. - Added the negative case.
parse_win_shim_target_rejects_an_npm_cmd_shim(sys.rs:2909) assertsNoneagainst an npmcmd-shimbody. - Also dropped a doc paragraph that had been duplicated on
unlink_bins.
I checked the npm fixture against the real writer rather than taking it on trust. npm/cmd-shim's writeShim_ builds its head byte-for-byte as the fixture reproduces it, and the sole %~dp0 in the whole template is SET dp0=%~dp0 followed by CRLF — never a backslash. Both the native-binary and node-interpreter branches build every path from the %dp0% variable, and the cmd-shim changelog shows no template change across 5.0.0 through 9.0.2. The fixture is faithful and the structural claim about npm holds.
Claude Opus ｜ 𝕏
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"`.
# Conflicts: # crates/nub-core/src/node/shim.rs # crates/nub-core/src/pm/shim.rs
There was a problem hiding this comment.
Note
ℹ️ One minor suggestion, inline.
Reviewed changes since 9a7082a:
7c044c3mergesorigin/main(5ff007f, #752 — "honorXDG_DATA_HOMEfor the shim dirs") with hand-resolved conflicts in exactly two files:crates/nub-core/src/node/shim.rsandcrates/nub-core/src/pm/shim.rs. No other file changed.
The resolution is correct. Both conflicts are the same shape — main rewrote the ShimBlock PATH lines to the XDG form while this PR changed the field types to Cow<'static, str> — and both were resolved by keeping main's line content inside the PR's Cow::Borrowed. Verified directionally: git diff 9a7082a1 7c044c37 over those two files shows exactly #752's contribution and nothing else, and git diff 5ff007f0 7c044c37 shows exactly this PR's contribution and nothing else. No hunk lost, no function duplicated. PM_SHIM_BLOCK correctly keeps main's deliberately dot-less dir_marker: "nub/shims", which is what lets strip_block still match a pre-move $HOME/.nub/shims line.
Also checked and clear:
- Compiles.
scripts/rust-build.sh check -p nub-core --profile fastandcheck -p nub-cli --all-targets --profile fastboth finish clean on the merged tree. EveryProfileOutcomematch site carries the newRewrittenarm; everyShimBlockconstruction usesCow. - The rewrite-on-upgrade interaction is a net improvement. This PR's
rewrite_marked_linenow rewrites the line beneath the marker for all families, and main just changed what that line says. I checked whether that could repoint an upgrading user at a directory the shims aren't in — it can't:resolve_shim_dir(pm/shim.rs:497) has deliberately no legacy branch andmigrate_legacy_shim_dirempties the old dir on install. Pre-PR, this same user would have gotten a second PATH block appended, so the merge strictly improves the upgrade path. aube-linker/link.rsis orthogonal. Main's only change there iscollect_hidden_hoist_packagesordering, which writes undernode_modules/.aube/; the ownership guard reads top-levelnode_modules/<alias>/and depends only onsys.rsfunctions main never touched.
The one thing worth a look is that the merge changed the truth-value of an existing comment and left a message under-informative for its new audience — see the inline note. Everything else in the delta is clean.
Claude Opus ｜ 𝕏
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`.
There was a problem hiding this comment.
ℹ️ Two minor suggestions, inline.
Reviewed changes since 7c044c3:
01beb15— "pm/node: print the re-source hint when an upgrade rewrites the PATH line". Four hunks: bothProfileOutcome::Rewrittenarms incli.rs, theRewrittendoc comment inpm/shim.rs, and one new test.
This addresses the previous thread, and the new comment is right where my last one was not. I claimed every pre-#752 user reaches this arm. The comment claims something narrower — that the arm is reached only when the legacy directory is already gone, because the migration strips the block outright while it still exists — and that is the correct reading. cli.rs:10244 calls migrate_legacy_shim_dir and, on Some, remove_path_block(); migrate_legacy_shim_dir (pm/shim.rs:649) returns Some only if legacy.exists(). So the live population is the synced-dotfile case: a profile carrying the pre-XDG line on a machine that never had ~/.nub/shims. The re-source hint is still the right call there — the line under the marker changed, and the running shell has not re-read it.
Also checked and clear:
- Both sites got the fix, and
diris in scope at each (shim_dir()?atcli.rs:10238,node_shim_dir()?at10424). - The new test fails for the right reason.
an_upgraded_profile_rewrites_the_pre_xdg_shims_lineseeds both.zshrcand.zshenv— correct, sinceAddedon any single target outranksRewrittenin the fold atshim.rs:1090and would mask the outcome. Pre-PR the same input appended a second block, so the marker-count assertion is what pins the behavior. Hermetic:tmpdiris unique per call andadd_path_block_forreads no environment.
The two notes below are a site this commit missed and a durability issue in the branch it just promoted to a live path.
Claude Opus ｜ 𝕏
`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.
|
Your Claude subscription has hit its usage limit. It resets at 11pm (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
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.
|
Your Claude subscription has hit its usage limit. It resets at 11pm (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
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.
There was a problem hiding this comment.
ℹ️ All three open threads are addressed. One note on the Windows retry that rode along, inline.
Reviewed changes since 01beb15 — four commits:
ad326c7—the_slot_policy_holds_for_plain_files_on_every_platform, un-gated, composingbin_slot_is_writablewith a real occupant. This retires the Windows-coverage thread: the third assertion writes throughaube_linker::create_bin_shimrather than a hand-written fixture, which is exactly the writer/parser disagreement that produced the last defect. Checked it compiles on thewindows-latestleg —tempfileis an unconditional dev-dep (aube/Cargo.toml:98,138) andBinShimOptions/create_bin_shimare re-exported without acfg(aube-linker/src/lib.rs:47,52). Leaving the sibling test#[cfg(unix)]is right; its store-shape fixture genuinely needsstd::os::unix::fs::symlink.3e60620—replace_profile_atomicallyextracted, and both writers routed through it, not just the one I flagged. The new unix test pins the symlink case on the rewrite path, which is what the strip-path test atshim.rs:2788never covered.install_family.rs:449takes the thirdRewrittenhint verbatim.38cccc5— the Windows rename retry. See the inline note.7fccb13— merge oforigin/main(18a7fd2). Diffed directionally against both parents: main's touches tocli.rs,aube-settings/settings.toml,aube-util/src/env.rsandcommands/config/*are disjoint from this PR's scope, and no hunk was lost either way. Bonus — main'ssettings.tomltemplating deleted the stalePNPM_HOME-fallback docs an earlier review flagged, so that item closes for free.
One thing I checked and am not raising: append_block's rewrite branch now sits above the may_create / PermissionDenied handling, so a rewrite on an unwritable profile hard-errors instead of degrading to Manual. That is not a regression — the pre-PR branch was std::fs::write(…)? and behaved identically.
ℹ️ Nitpicks
the_slot_policy_holds_for_plain_files_on_every_platformdoesn't test plain files on every platform.BinShimOptions::default()leavesprefer_symlinked_executablesasNone, whichcreate_bin_shimreads as "symlink on POSIX". So on unix the third assertion goes throughslot_entry_is_ours's symlink arm (global.rs:371-399), not the plain-file arm (400-420) — where it duplicates the#[cfg(unix)]test directly above it. The name is accurate only on the Windows leg. Passingprefer_symlinked_executables: Some(false)would make the name true everywhere; renaming it works too.38cccc5is unrelated to global bin ownership and unmentioned in the body. It fixes awindows-latestflake inconcurrent_exact_license_repairs_leave_one_attested_notice. Worth a line in the PR description, otherwise it's invisible to whoever bisects a license-repair regression later.
Claude Opus ｜ 𝕏
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.
There was a problem hiding this comment.
✅ No new issues found. The last open thread is closed, and the two external claims the new comment leans on both check out.
Reviewed changes since 7fccb13 — one commit, one file, +17/−6:
- Rebudgeted the Windows rename retry in time rather than attempts.
a6c4fbareplacesatomic_replace_file's 64-iterationyield_now()loop withCONTENTION_PAUSE = 1ms/CONTENTION_BUDGET = 500ms,contendedbecoming aDurationand the yield becoming asleep. That is what the open thread asked for, and it closes it.
I chased the one way this could still be hollow — whether contended accumulating a nominal 1 ms understates wall-clock, since Windows classically rounds a 1 ms sleep up to the ~15.6 ms system tick, which would quietly turn a 500 ms budget into ~7.8 s. It doesn't. rust-lang/rust#116461 ("Windows: Support sub-millisecond sleep") shipped in Rust 1.75.0, and library/std/src/sys/thread/windows.rs still drives sleep through CreateWaitableTimerExW with CREATE_WAITABLE_TIMER_HIGH_RESOLUTION at tag 1.97.1. The coarse fallback only engages below Windows 10 1803 — not in the win32-x64 / win32-arm64 matrix — and even that path is one Sleep plus at most one compensating Sleep(1), never a loop. With rust-version = "1.95" as the floor, the budget means what it says.
The comment's prior-art citation also holds: is_retryable_dir_error is a real symbol in rustup's src/utils/raw.rs, it does match PermissionDenied, and its caller remove_dir backs off with Fibonacci::from_millis(10) rather than yielding.
Also checked and clear: contended sits outside the loop, so it is a per-call total that is never reset; the NotFound arm above correctly keeps its unbounded continue; cfg!(windows) is a runtime-constant bool, so the two consts raise no dead-code warning on unix; and rustfmt --edition 2024 --check on the file exits 0.
ℹ️ Nitpicks
- The 500 ms budget is per call, not per repair.
atomic_write_node_license(crates/nub-core/src/version_management/mod.rs:718) callsatomic_replace_filethree times per iteration insidefor _ in 0..16, andcontendedis a local — so up to 48 independent budgets exist. A call that exhausts its budget returnsErrand unwinds through?, so the hard-failure path really is capped at one budget; the uncapped case is only calls that succeed after partially burning theirs, which under sustained contention could add up to roughly 24 s of sleeping. Not worth threading a shared deadline through for that, but the new comment reads as if it bounds the whole repair, and it bounds one rename.
Claude Opus ｜ 𝕏
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.
Closes #642
nub install -gexited 0 while linking bins into a pnpm-named path nothing puts on PATH. Also #708.XDG_BIN_HOME,$XDG_DATA_HOME/../bin, then~/.local/bin; packages to<data>/<ns>/global.PNPM_HOMEis no longer read.# nub global binmarker, so a relocation rewrites the line instead of appending.remove -gnow unlinks its bins: the ownership test never matched, because the isolated layout resolves them into the content store.install.shno longer re-adds its PATH block every run.tests/global-install/run.sh: 14 checks, 6 of which fail pre-fix.