From 08bb9c008f5fe459403d3e542dc3ed1fff2c49c4 Mon Sep 17 00:00:00 2001 From: Filip131311 Date: Sat, 1 Aug 2026 07:42:15 +0200 Subject: [PATCH] fix(installer): refuse an uninstall that cannot finish, before it destroys anything MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- packages/argent-installer/src/topology.ts | 76 +++++ packages/argent-installer/src/uninstall.ts | 148 +++++++- packages/argent-installer/src/utils.ts | 3 +- .../test/uninstall-permissions.test.ts | 165 +++++++++ .../test/uninstall-preflight.test.ts | 321 ++++++++++++++++++ .../argent-installer/test/uninstall.test.ts | 22 +- packages/registry/src/failure-codes.ts | 1 + scripts/e2e-full/phases/00-install.sh | 15 +- 8 files changed, 738 insertions(+), 13 deletions(-) create mode 100644 packages/argent-installer/test/uninstall-permissions.test.ts create mode 100644 packages/argent-installer/test/uninstall-preflight.test.ts diff --git a/packages/argent-installer/src/topology.ts b/packages/argent-installer/src/topology.ts index 36722ec81..ede428c4e 100644 --- a/packages/argent-installer/src/topology.ts +++ b/packages/argent-installer/src/topology.ts @@ -4,6 +4,7 @@ import { createRequire } from "node:module"; import { execSync } from "node:child_process"; import semver from "semver"; import { PACKAGE_NAME, MCP_BINARY_NAME } from "./constants.js"; +import { detectPackageManager } from "./package-manager.js"; import { resolvePackageRoot } from "./package-root.js"; import { isYarnPnp } from "./preflight.js"; @@ -85,6 +86,81 @@ export function getGloballyInstalledPackageRoot(): string | null { } } +/** Verdict of {@link probeGlobalPackageRemoval}. */ +export type RemovalWritability = "writable" | "blocked" | "unknown"; + +export interface GlobalRemovalProbe { + verdict: RemovalWritability; + /** Directory whose permissions decided the verdict; null unless "blocked". */ + parentDir: string | null; +} + +const UNKNOWN_REMOVAL: GlobalRemovalProbe = { verdict: "unknown", parentDir: null }; + +/** + * Can `npm uninstall -g` actually remove the global package as this user? + * + * `uninstall` prunes the workspace before it removes the package, so a removal + * that dies on permissions leaves the user with no config and a package that is + * still installed (issue #622). This answers the question BEFORE anything is + * deleted. + * + * npm does not unlink the package directory — it RENAMES it aside + * (`@swmansion/argent` -> `@swmansion/.argent-p3dt2fHx`), so the permission that + * decides the outcome belongs to the package's PARENT directory, not the package + * itself. + * + * Every inconclusive case returns "unknown" and callers must treat that exactly + * like "writable": a wrong "blocked" would refuse an uninstall that works, which + * is worse than the bug being fixed. + */ +export function probeGlobalPackageRemoval(): GlobalRemovalProbe { + // Windows `access(W_OK)` reflects only the read-only attribute, which is + // meaningless on a directory and carries no ACL signal — it would report + // "writable" for a directory the user cannot touch. Never guess there. + if (process.platform === "win32") return UNKNOWN_REMOVAL; + + // The rename-in-parent mechanic above is npm's. pnpm/yarn/bun globals live in + // their own stores and mutate different paths, so a reading taken here would + // not describe the command we are about to run. detectPackageManager() is the + // same function that BUILDS that command, so probe and command always agree. + if (detectPackageManager() !== "npm") return UNKNOWN_REMOVAL; + + if (process.getuid?.() === 0) return { verdict: "writable", parentDir: null }; + + const binaryPath = getGlobalBinaryPath(); + if (!binaryPath) return UNKNOWN_REMOVAL; + + // Deliberately NOT getGloballyInstalledPackageRoot(): that realpaths the bin, + // so under `npm link` it resolves to the source checkout and we would end up + // probing the checkout's parent — a directory npm never renames. That reads + // "blocked" whenever the checkout happens to be read-only, refusing an + // uninstall that would have succeeded. Derive the LOGICAL install path from + // the prefix instead, and bail on the symlink that marks a linked install. + const logicalPkgDir = path.join( + path.dirname(binaryPath), + "..", + "lib", + "node_modules", + PACKAGE_NAME + ); + const stat = fs.lstatSync(logicalPkgDir, { throwIfNoEntry: false }); + if (!stat || stat.isSymbolicLink()) return UNKNOWN_REMOVAL; + + const parentDir = path.dirname(logicalPkgDir); + try { + fs.accessSync(parentDir, fs.constants.W_OK); + return { verdict: "writable", parentDir }; + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + // Only a permission error is evidence. ENOENT and friends mean the layout + // is not what we assumed, which is a reason to stay quiet, not to block. + return code === "EACCES" || code === "EPERM" + ? { verdict: "blocked", parentDir } + : UNKNOWN_REMOVAL; + } +} + /** * Version of the globally-installed argent package — NOT the running package * ({@link import("./utils.js").getInstalledVersion}): under `npx` the running diff --git a/packages/argent-installer/src/uninstall.ts b/packages/argent-installer/src/uninstall.ts index e135e2e16..faacb3ec7 100644 --- a/packages/argent-installer/src/uninstall.ts +++ b/packages/argent-installer/src/uninstall.ts @@ -22,6 +22,7 @@ import { isDeclaredLocally, isGloballyInstalled, probeLocalInstall, + probeGlobalPackageRemoval, resolveInstallMode, removeInstallRecord, resolveProjectRoot, @@ -52,6 +53,16 @@ const UNINSTALL_PACKAGE_ACTION_FAILED: InstallerFailureSignal = { error_kind: "subprocess", }; +// The package could not be removed for an environmental reason and we stopped +// BEFORE touching anything. Distinct from UNINSTALL_PACKAGE_ACTION_FAILED, which +// means a removal actually ran and failed — no subprocess is involved here. +const UNINSTALL_PACKAGE_ROOT_NOT_WRITABLE: InstallerFailureSignal = { + error_code: FAILURE_CODES.UNINSTALL_PACKAGE_ROOT_NOT_WRITABLE, + failure_stage: "installer_uninstall_package_not_writable", + failure_area: "installer", + error_kind: "validation", +}; + // Catch-all for any unexpected throw in the prune/cleanup section or a prompt, // so the buffered cli_uninstall_start still flushes with a terminal event. const UNINSTALL_UNCLASSIFIED_FAILED: InstallerFailureSignal = { @@ -61,6 +72,33 @@ const UNINSTALL_UNCLASSIFIED_FAILED: InstallerFailureSignal = { error_kind: "unknown", }; +/** + * How to re-run an uninstall that this user lacks the permission to finish. + * + * Deliberately NOT `sudo -E`: Ubuntu 25.10 ships sudo-rs, which does not + * implement that flag at all — it prints "preserving the entire environment is + * not supported, `-E` is ignored" and HOME still becomes /root. The + * `sudo VAR=value cmd` form assigns the variable directly in the command's + * environment, so it survives `env_reset` on both sudo-rs and classic sudo. + * + * HOME has to survive, because the global-scope cleanup resolves ~/.claude, + * ~/.cursor and friends from it; under a reset HOME it would clean root's home + * and silently leave the user's own config in place. (Running as root does still + * create a root-owned ~/.argent for telemetry state, and rewrites any config + * file that keeps non-argent entries as root — unavoidable when the package + * itself lives in a root-owned prefix.) + * + * Re-running ARGENT rather than the package manager directly matters too: the + * package removal is only half the job, and 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. + */ +function elevatedRerunHint(): string { + return process.platform === "win32" + ? "Re-run this command from an elevated (Administrator) terminal." + : `Re-run it with elevated permissions: ${pc.cyan('sudo HOME="$HOME" argent uninstall --global')}`; +} + export interface BundledContentRemoval { removedPaths: string[]; removedRoot: boolean; @@ -358,6 +396,8 @@ export async function uninstall(args: string[]): Promise { let hasPrunedContent = false; let hasUninstalledPackage = false; let hasUninstalledGlobalPackage = false; + // First thing that went wrong, reported once after every target is attempted. + let firstFailure: InstallerFailureSignal | null = null; try { p.intro(pc.bgRed(pc.white(" argent uninstall "))); @@ -427,6 +467,64 @@ export async function uninstall(args: string[]): Promise { removeTargets = targetDecision.targets; } + // ── Preflight: can we actually remove the global package? ─────────────────── + // Everything below this point is destructive and irreversible, while the + // package removal at the very end can fail for a purely environmental reason + // (a root-owned npm prefix, the usual `sudo npm i -g` install). Running them + // in that order strips the workspace and then leaves the package installed — + // issue #622. Ask the environment first, while nothing has been touched. + // + // Only when the removal is already consented to: on a bare interactive run + // the per-install confirm below defaults to NO, and pruning-while-keeping the + // package is a supported outcome we must not turn into a hard failure. + const removalPreconsented = nonInteractive || removePreconfirmed; + let blockedGlobalRemoval = false; + // Blocked, but the user still gets asked (interactive, unconfirmed) — the + // prompt says so rather than letting them opt into a removal that will fail. + let globalRemovalNeedsElevation = false; + if (removeTargets.includes("global") && globalPresent) { + const probe = probeGlobalPackageRemoval(); + if (probe.verdict === "blocked") { + p.log.error( + `Cannot remove the global ${PACKAGE_NAME} package: ` + + `${pc.dim(probe.parentDir ?? "the install directory")} is not writable by this user, ` + + `so the removal would fail partway through.` + ); + p.log.info(elevatedRerunHint()); + if (removalPreconsented) { + blockedGlobalRemoval = true; + firstFailure = UNINSTALL_PACKAGE_ROOT_NOT_WRITABLE; + // Drop the target rather than aborting outright: a `--global --local` + // run can still remove the local devDependency. Dropping it also feeds + // the scope rule below, which then keeps the retained global install's + // config wired up instead of orphaning it. + removeTargets = removeTargets.filter((t) => t !== "global"); + } else { + // Interactive and unconfirmed: warn, but leave today's behavior intact. + // The prompt still asks, and still defaults to no. + globalRemovalNeedsElevation = true; + p.log.info( + pc.dim("Skipping the global package removal below will leave your setup as it is.") + ); + } + } + } + + if (blockedGlobalRemoval && removeTargets.length === 0) { + // Nothing left to do, and nothing has been modified yet. Stop here so the + // workspace configuration survives — it belongs to an install that is + // still on this machine. + p.log.info( + pc.dim( + "Nothing was changed by this run: argent is still installed, so its configuration " + + "was left in place." + ) + ); + await finalizeUninstallTelemetry(false, false, firstFailure ?? undefined); + p.outro(pc.red(`${PACKAGE_NAME} was not removed.`)); + process.exit(1); + } + // Which config scopes the entry/allowlist/content removal may touch: clean // everything EXCEPT the scopes that keep a RETAINED install wired up. A kept // global install keeps its global-scope entries (and, in global mode, its @@ -665,7 +763,9 @@ export async function uninstall(args: string[]): Promise { return { kind: "global", cmd: globalUninstallCommand(detectPackageManager(), PACKAGE_NAME), - prompt: `Uninstall the global ${PACKAGE_NAME} package?`, + prompt: globalRemovalNeedsElevation + ? `Uninstall the global ${PACKAGE_NAME} package? (will fail without elevated permissions)` + : `Uninstall the global ${PACKAGE_NAME} package?`, defaultRemove: false, installDir: getGloballyInstalledPackageRoot(), }; @@ -675,7 +775,10 @@ export async function uninstall(args: string[]): Promise { .map((t) => buildRemovable(t)) .filter((r): r is RemovableInstall => r !== null); - if (removables.length === 0) { + // Suppressed when the preflight dropped the global target: the install WAS + // detected, we just cannot remove it, and "no matching install detected" + // would contradict the reason we already printed. + if (removables.length === 0 && !blockedGlobalRemoval) { // The probe is PATH/node_modules based, so an install under a different // toolchain (or the other mode) is intentionally left untouched. p.log.info( @@ -724,16 +827,45 @@ export async function uninstall(args: string[]): Promise { p.log.info(pc.dim("Removed .argent/install.json (local mode marker).")); } } catch (err) { - p.log.error(`${removable.kind} uninstall failed: ${err}`); - await finalizeUninstallTelemetry( - hasPrunedContent, - hasUninstalledPackage, - UNINSTALL_PACKAGE_ACTION_FAILED + // The package manager's own output already streamed to the terminal + // (execShellCommandSync inherits stdio), so `err` here carries only + // "Command failed: " — there is no stderr to classify. Re-running + // the probe is what tells us whether this was a permission problem, and + // it needs no output parsing to do it. + const blockedNow = + removable.kind === "global" && probeGlobalPackageRemoval().verdict === "blocked"; + p.log.error( + blockedNow + ? `Removing the global ${PACKAGE_NAME} package failed: the install directory is ` + + `not writable by this user.` + : `${removable.kind} uninstall failed: ${err}` ); - return; + if (blockedNow) { + p.log.info(elevatedRerunHint()); + if (hasPrunedContent) { + p.log.warn( + pc.dim( + "Workspace configuration was already removed, but the package is still installed." + ) + ); + } + } + firstFailure ??= blockedNow + ? { ...UNINSTALL_PACKAGE_ACTION_FAILED, failure_spawn_code: "EACCES" } + : UNINSTALL_PACKAGE_ACTION_FAILED; + // Keep going: a failed global removal must not silently skip a local one + // the user also asked for, and finalizing here would report + // has_uninstalled_package=false even if a later target succeeds. + continue; } } + if (firstFailure) { + await finalizeUninstallTelemetry(hasPrunedContent, hasUninstalledPackage, firstFailure); + p.outro(pc.red(`${PACKAGE_NAME} was not fully removed — see above.`)); + process.exit(1); + } + await finalizeUninstallTelemetry(hasPrunedContent, hasUninstalledPackage); // Reset the machine-wide local telemetry state when the GLOBAL package was // removed, or when a removal left NO global install behind. NOT on a diff --git a/packages/argent-installer/src/utils.ts b/packages/argent-installer/src/utils.ts index f02b415dc..aeb7f1d92 100644 --- a/packages/argent-installer/src/utils.ts +++ b/packages/argent-installer/src/utils.ts @@ -42,8 +42,9 @@ export { readLocalPackageVersionUncached, getLocalArgentBinRelPath, probeLocalInstall, + probeGlobalPackageRemoval, } from "./topology.js"; -export type { LocalInstallProbe } from "./topology.js"; +export type { LocalInstallProbe, GlobalRemovalProbe, RemovalWritability } from "./topology.js"; export { getInstallRecordPath, readInstallRecord, diff --git a/packages/argent-installer/test/uninstall-permissions.test.ts b/packages/argent-installer/test/uninstall-permissions.test.ts new file mode 100644 index 000000000..9867964c8 --- /dev/null +++ b/packages/argent-installer/test/uninstall-permissions.test.ts @@ -0,0 +1,165 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; + +const execSyncMock = vi.hoisted(() => vi.fn(() => "")); +vi.mock("node:child_process", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, default: { ...actual, execSync: execSyncMock }, execSync: execSyncMock }; +}); + +import { probeGlobalPackageRemoval } from "../src/topology.js"; +import { PACKAGE_NAME } from "../src/constants.js"; + +/** + * The preflight behind issue #622: `uninstall` prunes the workspace before it + * removes the package, so a removal that dies on permissions leaves the user + * with no config and a package that is still installed. The probe answers + * "could the removal even work?" while nothing has been touched. + * + * The bug it prevents is destructive, but a WRONG probe is worse: a false + * "blocked" refuses an uninstall that would have succeeded. Hence the bias — + * everything inconclusive must read "unknown", which callers treat as + * "writable". + */ +describe("probeGlobalPackageRemoval", () => { + let tmpDir: string; + let originalAgent: string | undefined; + + /** + * The layout npm actually creates: a bin symlink into + * /lib/node_modules//dist. Returns the staged bin path. + */ + function stageInstall(root: string, opts: { linked?: boolean } = {}): string { + const pkgRoot = path.join(root, "lib", "node_modules", PACKAGE_NAME); + const binDir = path.join(root, "bin"); + fs.mkdirSync(binDir, { recursive: true }); + + if (opts.linked) { + // `npm link`: the package dir is a symlink to a source checkout. + const checkout = path.join(root, "checkout"); + fs.mkdirSync(path.join(checkout, "dist"), { recursive: true }); + fs.writeFileSync( + path.join(checkout, "package.json"), + JSON.stringify({ name: PACKAGE_NAME, version: "0.0.0" }) + ); + fs.mkdirSync(path.dirname(pkgRoot), { recursive: true }); + fs.symlinkSync(checkout, pkgRoot); + } else { + fs.mkdirSync(path.join(pkgRoot, "dist"), { recursive: true }); + fs.writeFileSync( + path.join(pkgRoot, "package.json"), + JSON.stringify({ name: PACKAGE_NAME, version: "0.0.0" }) + ); + } + + const binPath = path.join(binDir, "argent"); + fs.writeFileSync(binPath, "#!/usr/bin/env node\n"); + fs.chmodSync(binPath, 0o755); + return binPath; + } + + /** The scope directory npm renames inside — the one whose mode decides it. */ + function scopeDir(root: string): string { + return path.dirname(path.join(root, "lib", "node_modules", PACKAGE_NAME)); + } + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "argent-removal-probe-")); + originalAgent = process.env.npm_config_user_agent; + process.env.npm_config_user_agent = "npm/10.0.0 node/v22.0.0"; + execSyncMock.mockReset(); + }); + + afterEach(() => { + if (originalAgent === undefined) delete process.env.npm_config_user_agent; + else process.env.npm_config_user_agent = originalAgent; + // Restore write permission first or the cleanup itself throws. + try { + fs.chmodSync(scopeDir(tmpDir), 0o755); + } catch { + // Never staged, or already writable. + } + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + const skipUnlessPosixUser = process.platform === "win32" || process.getuid?.() === 0; + + it.skipIf(skipUnlessPosixUser)("reports blocked when the scope dir is not writable", () => { + const binPath = stageInstall(tmpDir); + execSyncMock.mockReturnValue(`${binPath}\n`); + fs.chmodSync(scopeDir(tmpDir), 0o555); + + const probe = probeGlobalPackageRemoval(); + + expect(probe.verdict).toBe("blocked"); + expect(probe.parentDir).toBe(scopeDir(tmpDir)); + }); + + it.skipIf(skipUnlessPosixUser)("reports writable for the same layout at 0o755", () => { + const binPath = stageInstall(tmpDir); + execSyncMock.mockReturnValue(`${binPath}\n`); + + expect(probeGlobalPackageRemoval().verdict).toBe("writable"); + }); + + it.skipIf(skipUnlessPosixUser)( + "stays unknown for a linked install, whose parent npm never renames", + () => { + // Under `npm link` the resolved package root is the source checkout, so a + // realpath-based probe would measure the checkout's parent — a directory + // npm does not touch. If that checkout happened to be read-only we would + // refuse an uninstall that works. Bail on the symlink instead. + const binPath = stageInstall(tmpDir, { linked: true }); + execSyncMock.mockReturnValue(`${binPath}\n`); + fs.chmodSync(scopeDir(tmpDir), 0o555); + + expect(probeGlobalPackageRemoval().verdict).toBe("unknown"); + } + ); + + it("stays unknown when argent is not on PATH", () => { + execSyncMock.mockImplementation(() => { + throw new Error("which: no argent"); + }); + + expect(probeGlobalPackageRemoval().verdict).toBe("unknown"); + }); + + it.skipIf(skipUnlessPosixUser)("stays unknown when the package dir is absent", () => { + const binDir = path.join(tmpDir, "bin"); + fs.mkdirSync(binDir, { recursive: true }); + const binPath = path.join(binDir, "argent"); + fs.writeFileSync(binPath, "#!/usr/bin/env node\n"); + execSyncMock.mockReturnValue(`${binPath}\n`); + + // Nothing at the logical path: the layout is not what we assumed, which is + // a reason to stay quiet rather than to block. + expect(probeGlobalPackageRemoval().verdict).toBe("unknown"); + }); + + it.skipIf(skipUnlessPosixUser)( + "stays unknown under a non-npm manager even when the dir is unwritable", + () => { + // pnpm/yarn/bun globals live in their own stores; the rename-in-parent + // mechanic this probe measures is npm's alone. + const binPath = stageInstall(tmpDir); + execSyncMock.mockReturnValue(`${binPath}\n`); + fs.chmodSync(scopeDir(tmpDir), 0o555); + process.env.npm_config_user_agent = "pnpm/9.0.0 node/v22.0.0"; + + expect(probeGlobalPackageRemoval().verdict).toBe("unknown"); + } + ); + + it.skipIf(process.platform !== "win32")("stays unknown on Windows", () => { + // access(W_OK) there reflects only the read-only attribute, which says + // nothing about a directory's ACL — it would read "writable" for a dir the + // user cannot touch. + const binPath = stageInstall(tmpDir); + execSyncMock.mockReturnValue(`${binPath}\n`); + + expect(probeGlobalPackageRemoval().verdict).toBe("unknown"); + }); +}); diff --git a/packages/argent-installer/test/uninstall-preflight.test.ts b/packages/argent-installer/test/uninstall-preflight.test.ts new file mode 100644 index 000000000..6f76e85f4 --- /dev/null +++ b/packages/argent-installer/test/uninstall-preflight.test.ts @@ -0,0 +1,321 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import * as fs from "node:fs"; +import * as path from "node:path"; +import * as os from "node:os"; +import { uninstall } from "../src/uninstall.js"; + +/** + * Issue #622: `uninstall` prunes the workspace and only then runs + * `npm uninstall -g`. When the global prefix is root-owned (the usual + * `sudo npm i -g` install) that removal dies on EACCES, and the user is left + * with no configuration and a package that is still installed — and, before + * this fix, an exit code of 0 saying it all went fine. + * + * These tests pin the contract that makes that impossible: nothing irreversible + * runs until we know the removal can work, and a run that did not finish says so + * to the shell. + */ + +const telemetryMock = vi.hoisted(() => ({ + init: vi.fn(), + track: vi.fn(), + resetLocalTelemetryState: vi.fn().mockResolvedValue({ localIdRemoved: true, noticeReset: true }), + shutdown: vi.fn().mockResolvedValue(undefined), + warmTelemetryIdentitySync: vi.fn(), +})); + +const childProcessMock = vi.hoisted(() => ({ + execSync: vi.fn(() => "/usr/local/bin/argent\n"), + execFileSync: vi.fn(), + spawn: vi.fn(), +})); + +const promptsMock = vi.hoisted(() => ({ + intro: vi.fn(), + outro: vi.fn(), + cancel: vi.fn(), + confirm: vi.fn(async () => true), + multiselect: vi.fn(), + isCancel: vi.fn(() => false), + note: vi.fn(), + log: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + message: vi.fn(), + step: vi.fn(), + success: vi.fn(), + }, + spinner: vi.fn(() => ({ start: vi.fn(), stop: vi.fn() })), +})); + +vi.mock("@argent/telemetry", () => telemetryMock); +vi.mock("node:child_process", () => childProcessMock); +vi.mock("@clack/prompts", () => promptsMock); +vi.mock("@argent/tools-client", () => ({ + killToolServer: vi.fn().mockResolvedValue(undefined), + killToolServerForInstallDir: vi.fn().mockResolvedValue(0), +})); +vi.mock("../src/telemetry-finalize.js", () => ({ + finalizeTelemetry: vi.fn(async (capture: () => void) => capture()), +})); + +// The probe reads real filesystem permissions; force its verdict here so these +// tests describe the FLOW rather than the machine they run on. The probe's own +// behavior is covered in uninstall-permissions.test.ts. +const probeState = vi.hoisted(() => ({ + verdict: "writable" as "writable" | "blocked" | "unknown", +})); + +vi.mock("../src/utils.js", async (importOriginal) => { + const original = await importOriginal(); + return { + ...original, + isGloballyInstalled: vi.fn(() => true), + probeGlobalPackageRemoval: vi.fn(() => ({ + verdict: probeState.verdict, + parentDir: "/usr/local/lib/node_modules/@swmansion", + })), + }; +}); + +class ExitSentinel extends Error { + constructor(public readonly code: number | undefined) { + super(`process.exit(${code})`); + } +} + +let tmpDir: string; +let projDir: string; +let originalCwd: string; +let savedHome: string | undefined; +let savedUserProfile: string | undefined; +let exitSpy: ReturnType; + +/** Workspace content the prune would destroy, staged so we can assert it survives. */ +function stageWorkspaceConfig(): { mcpPath: string; skillPath: string } { + const mcpPath = path.join(projDir, ".mcp.json"); + fs.writeFileSync( + mcpPath, + JSON.stringify({ mcpServers: { argent: { command: "argent", args: ["mcp"] } } }, null, 2) + ); + const skillDir = path.join(projDir, ".agents", "skills", "argent-device-interact"); + fs.mkdirSync(skillDir, { recursive: true }); + const skillPath = path.join(skillDir, "SKILL.md"); + fs.writeFileSync(skillPath, "# argent-device-interact\n"); + return { mcpPath, skillPath }; +} + +function npmUninstallCalls(): unknown[][] { + return childProcessMock.execFileSync.mock.calls.filter((call) => { + const args = call[1]; + return Array.isArray(args) && args.includes("uninstall"); + }) as unknown[][]; +} + +function loggedText(): string { + const all = [ + ...promptsMock.log.error.mock.calls, + ...promptsMock.log.info.mock.calls, + ...promptsMock.log.warn.mock.calls, + ]; + return all.map((call) => String(call[0])).join("\n"); +} + +beforeEach(() => { + vi.clearAllMocks(); + probeState.verdict = "writable"; + childProcessMock.execSync.mockReturnValue("/usr/local/bin/argent\n"); + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "argent-uninstall-preflight-")); + originalCwd = process.cwd(); + savedHome = process.env.HOME; + savedUserProfile = process.env.USERPROFILE; + process.env.HOME = tmpDir; + process.env.USERPROFILE = tmpDir; + projDir = path.join(tmpDir, "proj"); + fs.mkdirSync(projDir, { recursive: true }); + fs.writeFileSync(path.join(projDir, "package.json"), JSON.stringify({ name: "proj" })); + fs.mkdirSync(path.join(projDir, ".git"), { recursive: true }); + process.chdir(projDir); + exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new ExitSentinel(code); + }) as never); +}); + +afterEach(() => { + exitSpy.mockRestore(); + process.chdir(originalCwd); + if (savedHome === undefined) delete process.env.HOME; + else process.env.HOME = savedHome; + if (savedUserProfile === undefined) delete process.env.USERPROFILE; + else process.env.USERPROFILE = savedUserProfile; + fs.rmSync(tmpDir, { recursive: true, force: true }); +}); + +describe("uninstall preflight — a removal that cannot work destroys nothing", () => { + it("keeps the workspace intact and exits 1 when the global prefix is not writable", async () => { + probeState.verdict = "blocked"; + const { mcpPath, skillPath } = stageWorkspaceConfig(); + const mcpBefore = fs.readFileSync(mcpPath, "utf8"); + + await expect(uninstall(["--yes"])).rejects.toThrow(ExitSentinel); + + // The whole point: the config belongs to an install that is still here. + expect(fs.existsSync(mcpPath)).toBe(true); + expect(fs.readFileSync(mcpPath, "utf8")).toBe(mcpBefore); + expect(fs.existsSync(skillPath)).toBe(true); + // And we never even asked the package manager. + expect(npmUninstallCalls()).toHaveLength(0); + expect(exitSpy).toHaveBeenCalledWith(1); + }); + + it("reports the blocked preflight as its own failure, not as a failed subprocess", async () => { + probeState.verdict = "blocked"; + + await expect(uninstall(["--yes"])).rejects.toThrow(ExitSentinel); + + expect(telemetryMock.track).toHaveBeenCalledWith( + "installation:cli_uninstall_complete", + expect.objectContaining({ + error_code: "UNINSTALL_PACKAGE_ROOT_NOT_WRITABLE", + has_pruned_content: false, + has_uninstalled_package: false, + }) + ); + // Nothing was removed, so the machine-wide telemetry identity stays put. + expect(telemetryMock.resetLocalTelemetryState).not.toHaveBeenCalled(); + }); + + it("tells the user how to finish the job", async () => { + probeState.verdict = "blocked"; + + await expect(uninstall(["--yes"])).rejects.toThrow(ExitSentinel); + + const text = loggedText(); + expect(text).toMatch(/not writable/); + if (process.platform === "win32") { + expect(text).toMatch(/Administrator/); + } else { + // `sudo -E` is NOT usable: sudo-rs (Ubuntu 25.10's default) ignores the + // flag outright, leaving HOME as /root so the user's own global config + // would be missed. The VAR=value form survives env_reset everywhere. + expect(text).toContain('sudo HOME="$HOME" argent uninstall --global'); + expect(text).not.toContain("sudo -E"); + } + }); + + it("does not claim the install was never found — it was found, just not removable", async () => { + probeState.verdict = "blocked"; + + await expect(uninstall(["--yes"])).rejects.toThrow(ExitSentinel); + + expect(loggedText()).not.toMatch(/no matching .* install detected/); + }); +}); + +describe("uninstall preflight — inconclusive readings must not block", () => { + it('proceeds normally when the probe says "unknown"', async () => { + // ACLs, exotic mounts, non-npm managers and Windows all land here. Treating + // "unknown" as "blocked" would refuse uninstalls that work today. + probeState.verdict = "unknown"; + stageWorkspaceConfig(); + + await uninstall(["--yes"]); + + expect(npmUninstallCalls().length).toBeGreaterThan(0); + expect(exitSpy).not.toHaveBeenCalled(); + }); + + it('proceeds normally when the probe says "writable"', async () => { + stageWorkspaceConfig(); + + await uninstall(["--yes"]); + + expect(npmUninstallCalls().length).toBeGreaterThan(0); + expect(exitSpy).not.toHaveBeenCalled(); + }); +}); + +describe("uninstall preflight — scoped to the target actually being removed", () => { + it("leaves a --local run alone even when the global prefix is unwritable", async () => { + // A user-owned devDependency has nothing to do with the global prefix. + probeState.verdict = "blocked"; + const nodeModules = path.join(projDir, "node_modules", "@swmansion", "argent"); + fs.mkdirSync(nodeModules, { recursive: true }); + fs.writeFileSync( + path.join(nodeModules, "package.json"), + JSON.stringify({ name: "@swmansion/argent", version: "0.0.0" }) + ); + fs.writeFileSync( + path.join(projDir, "package.json"), + JSON.stringify({ name: "proj", devDependencies: { "@swmansion/argent": "0.0.0" } }) + ); + + await uninstall(["--yes", "--local"]); + + expect(exitSpy).not.toHaveBeenCalled(); + // Whatever ran, it was not a global removal. + for (const call of npmUninstallCalls()) { + expect(call[1]).not.toContain("-g"); + } + }); +}); + +describe("uninstall — a failed removal is reported to the shell", () => { + it("exits 1 when the package manager fails, instead of reporting success", async () => { + // The reported symptom's other half: before this fix the command returned 0 + // after a destructive, half-finished run, so `argent uninstall -y && …` + // carried on as though the package were gone. + childProcessMock.execFileSync.mockImplementation((bin: string) => { + if (bin === "npm") throw new Error("Command failed: npm uninstall -g @swmansion/argent"); + return undefined; + }); + + await expect(uninstall(["--yes"])).rejects.toThrow(ExitSentinel); + + expect(exitSpy).toHaveBeenCalledWith(1); + }); + + it("names the permission problem when the removal fails on an unwritable prefix", async () => { + // Nothing to classify from: execShellCommandSync inherits stdio, so npm's + // stderr went to the terminal and never reaches us. Re-probing is what + // identifies this without parsing any output. + let probeCalls = 0; + probeState.verdict = "writable"; + childProcessMock.execFileSync.mockImplementation((bin: string) => { + if (bin === "npm") { + // The prefix turns out to be unwritable only once npm tries. + probeState.verdict = "blocked"; + probeCalls++; + throw new Error("Command failed: npm uninstall -g @swmansion/argent"); + } + return undefined; + }); + + await expect(uninstall(["--yes"])).rejects.toThrow(ExitSentinel); + + expect(probeCalls).toBe(1); + expect(loggedText()).toMatch(/not writable/); + expect(telemetryMock.track).toHaveBeenCalledWith( + "installation:cli_uninstall_complete", + expect.objectContaining({ + error_code: "UNINSTALL_PACKAGE_ACTION_FAILED", + failure_spawn_code: "EACCES", + }) + ); + }); + + it("passes other failures through unchanged", async () => { + probeState.verdict = "writable"; + childProcessMock.execFileSync.mockImplementation((bin: string) => { + if (bin === "npm") throw new Error("Command failed: ENOTFOUND registry.npmjs.org"); + return undefined; + }); + + await expect(uninstall(["--yes"])).rejects.toThrow(ExitSentinel); + + const text = loggedText(); + expect(text).toMatch(/ENOTFOUND/); + expect(text).not.toMatch(/not writable/); + }); +}); diff --git a/packages/argent-installer/test/uninstall.test.ts b/packages/argent-installer/test/uninstall.test.ts index 6cc2c035f..7f5b38543 100644 --- a/packages/argent-installer/test/uninstall.test.ts +++ b/packages/argent-installer/test/uninstall.test.ts @@ -51,10 +51,22 @@ vi.mock("@clack/prompts", () => ({ message: vi.fn(), step: vi.fn(), success: vi.fn(), + warn: vi.fn(), }, note: vi.fn(), })); +// The uninstall preflight asks the real filesystem whether the global npm +// prefix is writable. Left unmocked, this suite's verdict would 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 or the reporter's layout. Pin it; the probe's own behavior is covered in +// utils.test.ts. +vi.mock("../src/utils.js", async (importOriginal) => ({ + ...(await importOriginal()), + probeGlobalPackageRemoval: () => ({ verdict: "writable", parentDir: null }), +})); + let tmpDir: string; let originalCwd: string; @@ -151,8 +163,16 @@ describe("uninstall — telemetry consent preservation", () => { if (bin === "npm") throw new Error("npm failed"); return undefined; }); + const exitSpy = vi.spyOn(process, "exit").mockImplementation((code) => { + throw new Error(`exit:${code}`); + }); - await uninstall(["--yes"]); + // A failed package removal must not report success to the shell — a script + // chaining `argent uninstall -y && …` would otherwise carry on as if the + // package were gone. This is the exit-code half of issue #622. + await expect(uninstall(["--yes"])).rejects.toThrow("exit:1"); + expect(exitSpy).toHaveBeenCalledWith(1); + exitSpy.mockRestore(); expect(telemetryMock.track).toHaveBeenCalledWith( "installation:cli_uninstall_complete", diff --git a/packages/registry/src/failure-codes.ts b/packages/registry/src/failure-codes.ts index 598a080a8..2315dfefe 100644 --- a/packages/registry/src/failure-codes.ts +++ b/packages/registry/src/failure-codes.ts @@ -46,6 +46,7 @@ export const FAILURE_CODES = { UNINSTALL_TOOLSERVER_STOP_FAILED: "UNINSTALL_TOOLSERVER_STOP_FAILED", UNINSTALL_PACKAGE_ACTION_FAILED: "UNINSTALL_PACKAGE_ACTION_FAILED", + UNINSTALL_PACKAGE_ROOT_NOT_WRITABLE: "UNINSTALL_PACKAGE_ROOT_NOT_WRITABLE", UNINSTALL_UNCLASSIFIED_FAILED: "UNINSTALL_UNCLASSIFIED_FAILED", VEGA_CLI_COMMAND_FAILED: "VEGA_CLI_COMMAND_FAILED", diff --git a/scripts/e2e-full/phases/00-install.sh b/scripts/e2e-full/phases/00-install.sh index 1725e833a..dd837629d 100644 --- a/scripts/e2e-full/phases/00-install.sh +++ b/scripts/e2e-full/phases/00-install.sh @@ -160,10 +160,19 @@ run_phase() { # --- uninstall GLOBAL, then restore the driver so downstream phases run --- pushd "$gws" >/dev/null - if argent_cli uninstall --yes --global; then pass "$P" uninstall-global exit0; else skip "$P" uninstall-global exit0 "$(printf '%s' "$CLI_OUT" | tail -2 | tr '\n' ' ')"; fi + local uninstalled_global=0 + if argent_cli uninstall --yes --global; then uninstalled_global=1; pass "$P" uninstall-global exit0; else skip "$P" uninstall-global exit0 "$(printf '%s' "$CLI_OUT" | tail -2 | tr '\n' ' ')"; fi popd >/dev/null - local gcfg2; gcfg2="$(_argent_mcp_in_ws "$gws")" - if [ -z "$gcfg2" ]; then pass "$P" uninstall-global config-removed; else fail "$P" uninstall-global config-removed "argent MCP config remains: $gcfg2"; fi + # Only assert the config is gone when the uninstall actually ran. It refuses, + # by design, when the npm prefix is not writable by this user (a root-owned + # `sudo npm i -g` prefix), and in that case leaving the config in place is the + # correct outcome — the package is still installed and still needs it. + if [ "$uninstalled_global" -eq 0 ]; then + skip "$P" uninstall-global config-removed "uninstall did not run" + else + local gcfg2; gcfg2="$(_argent_mcp_in_ws "$gws")" + if [ -z "$gcfg2" ]; then pass "$P" uninstall-global config-removed; else fail "$P" uninstall-global config-removed "argent MCP config remains: $gcfg2"; fi + fi # Restore the global driver (uninstall --global removes the sandbox bin that # ARGENT_BIN points at). Fast: the tarball is local and npm caches it.