fix(installer): refuse an uninstall that cannot finish, before it destroys anything - #679
Open
filip131311 wants to merge 1 commit into
Open
fix(installer): refuse an uninstall that cannot finish, before it destroys anything#679filip131311 wants to merge 1 commit into
filip131311 wants to merge 1 commit into
Conversation
…troys anything `argent uninstall` pruned the workspace first and ran `npm uninstall -g` last. On the common `sudo npm i -g @swmansion/argent` layout the removal dies on EACCES, so the user was left with no configuration, a package that is still installed, ~35 lines of raw npm stack — and an exit code of 0 saying it all went fine. Ask the environment before touching anything. npm does not unlink the package directory, it renames it aside within its parent, so a single access(W_OK) on that parent decides the outcome; measured against the reported layout it reads false unelevated and true as root. The probe is biased hard toward silence: Windows, any non-npm global manager, a linked install and every inconclusive stat return "unknown", which callers treat exactly like "writable". A wrong "blocked" would refuse an uninstall that works, which is worse than the bug being fixed. It also derives the install path logically from the prefix rather than realpathing the bin, so an `npm link` checkout is never mistaken for the directory npm mutates. Blocking only applies where the removal is already consented to (-y, --global, or the coexistence prompt). A bare interactive run still asks, still defaults to no, and pruning-while-keeping-the-package stays a supported outcome — it just says up front that the removal needs elevation. Reordering (package first, then prune) looks like the obvious fix and is not safe: the prune reads SKILLS_DIR/RULES_DIR/AGENTS_DIR off the running package at prune time, so removing that package first turns the whole prune into a silent no-op that orphans every installed skill, rule and agent. Two things fall out of the same bug: - A failed removal now exits 1. It returned 0 while the sibling tool-server failure in the same function threw, so this was an inconsistency rather than a contract; nothing in the repo chains `argent uninstall`. - A removal that fails anyway is classified by re-running the probe, not by parsing output — execShellCommandSync inherits stdio, so npm's stderr goes straight to the terminal and never reaches the catch. That also means no stdio change was needed, and `update` is untouched. The remedy line is `sudo HOME="$HOME" argent uninstall --global`, not `sudo -E`: Ubuntu 25.10 ships sudo-rs, which does not implement -E at all and leaves HOME as /root, which would clean root's home and silently miss the user's own config. The VAR=value form survives env_reset on both implementations. Windows gets an Administrator-terminal hint instead. Also stops the e2e global-uninstall phase from hard-failing when the uninstall correctly refuses: it asserted the config was gone regardless of whether the uninstall ran. Verified end to end on a Linux VM matching the report (npm prefix /usr/local, root-owned): the blocked run leaves all 16 staged skills in place and exits 1, and the suggested command then removes the package and cleans the workspace. Fixes #622
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #622.
argent uninstallprunes the workspace first and removes the package last. On the layout the issue reports —sudo npm i -g @swmansion/argent, npm prefix/usr/local, root-ownednode_modules— the removal dies on EACCES after the pruning is already done.Reproduced
On a Linux VM matching the report exactly:
And it exits 0. A failed, destructive, half-finished uninstall reports success to the shell, so
argent uninstall -y && …carries on as though the package were gone. That is not in the issue; I found it while reproducing. It reads as an oversight rather than a contract — the sibling tool-server failure in the same function throws (uninstall.ts:709), and nothing in this repo chainsargent uninstall.The fix
Ask the environment before touching anything. npm doesn't unlink the package directory, it renames it aside within its parent (
@swmansion/argent→@swmansion/.argent-p3dt2fHx), so oneaccess(W_OK)on that parent decides the outcome. Measured on the reported layout:falseunelevated,trueas root.The probe is biased hard toward silence. Windows (where
access(W_OK)reflects only the read-only attribute and would answer "writable" for a directory you cannot touch), any non-npm global manager, a linked install, and every inconclusive stat all return"unknown", which callers treat exactly like"writable". A wrong"blocked"refuses an uninstall that works, which is worse than the bug being fixed.It also derives the install path logically from the prefix instead of realpathing the bin. Under
npm linkthe realpath lands in the source checkout, and probing that checkout's parent — a directory npm never renames — would produce a false block whenever the checkout happened to be read-only.Blocking applies only where the removal is already consented to (
-y,--global, or the coexistence prompt). A bare interactive run still asks, still defaults to no, and pruning-while-keeping-the-package remains a supported outcome — the prompt just says up front that the removal needs elevation.Why not simply reorder
Removing the package first is the obvious fix and it is unsafe. The prune reads
SKILLS_DIR/RULES_DIR/AGENTS_DIRoff the running package at prune time (getBundledSkillNames→[]on a missing dir,uninstall.ts:182;removeBundledContent→ empty,:112-114). When the running copy is the global install, npm renames that tree away and the entire prune becomes a silent no-op that orphans every installed skill, rule and agent. Making the reorder safe needs snapshotting the manifests and a signature change — a much larger, riskier change than this.Two things that fall out of the same bug
execShellCommandSyncinherits stdio, so npm's stderr goes to the terminal and the caught error carries only"Command failed: …"— there is nothing to regex. A removal that fails anyway is classified by re-running the probe. That also means no stdio change was needed, soupdateis untouched and npm's live output is unchanged.The remedy line
sudo HOME="$HOME" argent uninstall --global, deliberately notsudo -E. Ubuntu 25.10 ships sudo-rs, which does not implement-Eat all:A reset
HOMEwould make the global-scope cleanup clean root's home and silently miss the user's own~/.claude,~/.cursorand friends. Thesudo VAR=valueform assigns the variable directly and survivesenv_reseton both sudo implementations. Windows gets an Administrator-terminal hint instead.Re-running argent rather than npm directly matters too: removing the package by hand strands every MCP entry, skill and rule pointing at a binary that is gone, with argent no longer around to clean them up.
Verified live, end to end
Same VM, same workspace, fix grafted into the installed package:
Then the suggested command, from that same state:
Tests
uninstall-permissions.test.tscovers the probe (blocked / writable / linked-install / not-on-PATH / missing dir / non-npm manager / Windows).uninstall-preflight.test.tscovers the flow: workspace byte-identical and no package-manager call on a blocked run, its own telemetry code, the remedy text, fail-open on"unknown"and"writable",--localunaffected by an unwritable global prefix, exit 1 on failure, and permission-vs-other-failure classification.Both halves are mutation-verified — disabling the gate fails 4 tests, reverting the exit code fails 4 more.
Two existing-suite changes were required:
uninstall.test.ts:148drives exactly the path that now exits 1 (it becomes the regression guard for the reported bug), and its probe is pinned so the suite's result cannot depend on whether the machine running it happens to have a root-owned/usr/local/lib/node_modules/@swmansion— green on nvm, red on a stock Linux box.Also fixes the e2e global-uninstall phase, which asserted the config was gone regardless of whether the uninstall actually ran (
00-install.sh:165) and would hard-fail when the uninstall correctly refuses.Not in scope
updatehas the same unmapped-EACCES gap. Not destructive in the same way (nothing is pruned first), so it belongs in its own change..claude/skillssymlinks — does not reproduce on 0.18.0 (skills are real directories under.agents/skills), so I did not claim it.uninstalltrusts the exit code, unlike install/update which decide from disk. A post-removalisGloballyInstalled()check would catch "npm exited 0 but the package is still there". Left alone here.