fix(scripts): remove all detected installations on uninstall

scripts/install_tool.sh

@@ -90,7 +90,9 @@ if [ "$ACTION" = "uninstall" ]; then
       echo "[$TOOL] Skipping system binary: $path (managed by OS)" >&2
       continue
     fi
-    remove_installation "$TOOL" "$base_method" "$binary_name"
+    # || true: one failed removal (e.g. an unsupported method) must not abort
+    # the remaining removals under set -e; leftovers surface in the verify below
+    remove_installation "$TOOL" "$base_method" "$binary_name" "$path" || true
   done
 
   # Verify removal (ignore system entries in the check)

scripts/lib/capability.sh

@@ -49,7 +49,12 @@
       return 0
       ;;
     "$HOME/.nvm/"*)
-      echo "nvm"
+      # Only the node runtime itself is nvm-managed; anything else inside an
+      # nvm node dir is an npm global package (removable via npm uninstall -g)
+      case "$tool" in
+        node|npm|npx|corepack) echo "nvm" ;;
+        *) echo "npm" ;;
+      esac
       return 0
       ;;
     "/usr/local/bin/"*)
@@ -65,7 +70,7 @@
       # Check if it's an npm global installed under brew's node (symlink to lib/node_modules/...)
       local resolved_brew
       resolved_brew="$(readlink -f "$binary_path" 2>/dev/null || true)"
       if [ -n "$resolved_brew" ] && [[ "$resolved_brew" == */node_modules/* ]]; then
         echo "npm"
       elif command -v brew >/dev/null 2>&1 && brew list --formula 2>/dev/null | grep -q "^${tool}\$"; then
         echo "brew"
@@ -82,13 +87,13 @@
       # Check if it's a corepack shim
       local resolved_bp
       resolved_bp="$(readlink -f "$binary_path" 2>/dev/null || true)"
       if [ -n "$resolved_bp" ] && [[ "$resolved_bp" == */corepack/* ]]; then
         echo "corepack"
         return 0
       fi
       # Check if it's an apt package
       if command -v dpkg >/dev/null 2>&1; then
         if dpkg -S "$binary_path" >/dev/null 2>&1; then
           echo "apt"
           return 0
         fi
@@ -130,11 +135,11 @@
   while IFS= read -r line; do
     # Parse "binary is /path/to/binary" format
     local path="${line##* is }"
     [ -z "$path" ] && continue
     [ ! -x "$path" ] && continue
 
     # Skip venv/virtualenv paths - these are environments, not installations
     case "$path" in
       */venv/bin/*|*/.venv/bin/*|*/venvs/*/bin/*|*/.venvs/*/bin/*|*/virtualenvs/*/bin/*|*/.virtualenvs/*/bin/*) continue ;;
       */envs/*/bin/*|*/conda/*/bin/*|*/miniconda*/bin/*|*/anaconda*/bin/*) continue ;;
       */env/bin/*) continue ;;
@@ -144,7 +149,7 @@
     # Resolve symlinks for deduplication (e.g., /bin/X and /usr/bin/X are the same on Ubuntu)
     local resolved_path
     resolved_path="$(readlink -f "$path" 2>/dev/null || realpath "$path" 2>/dev/null || echo "$path")"
     [ -n "${seen_paths[$resolved_path]:-}" ] && continue
     seen_paths[$resolved_path]=1
 
     # Classify the installation method based on path
@@ -186,7 +191,12 @@
       # Extract node version for context
       local node_version="${path#$HOME/.nvm/versions/node/}"
       node_version="${node_version%%/*}"
-      echo "nvm($node_version)"
+      # Only the node runtime itself is nvm-managed; anything else inside an
+      # nvm node dir is an npm global package (removable via npm uninstall -g)
+      case "$tool" in
+        node|npm|npx|corepack) echo "nvm($node_version)" ;;
+        *) echo "npm($node_version)" ;;
+      esac
       ;;
     "$HOME/.local/pipx/venvs/"*)
       local pkg="${path#$HOME/.local/pipx/venvs/}"
@@ -197,7 +207,7 @@
       # Check if it's an npm global installed under brew's node (symlink to lib/node_modules/...)
       local resolved_brew
       resolved_brew="$(readlink -f "$path" 2>/dev/null || true)"
       if [ -n "$resolved_brew" ] && [[ "$resolved_brew" == */node_modules/* ]]; then
         echo "npm"
       else
         echo "brew"
@@ -214,7 +224,7 @@
       # Check if it's a corepack shim (symlink to corepack dist)
       local resolved
       resolved="$(readlink -f "$path" 2>/dev/null || true)"
       if [ -n "$resolved" ] && [[ "$resolved" == */corepack/* ]]; then
         echo "corepack"
       elif command -v dpkg >/dev/null 2>&1 && dpkg -S "$path" >/dev/null 2>&1; then
         echo "apt"
@@ -251,7 +261,7 @@
   local binary="${2:-$tool}"
   local count
   count="$(count_installations "$tool" "$binary")"
   [ "$count" -gt 1 ]
 }
 
 # Check if an installation method is available on this system
@@ -267,7 +277,7 @@
         return 1
       fi
       # Check sudo access (try non-interactively)
       if [ "$(id -u)" -eq 0 ]; then
         return 0  # root user
       fi
       if sudo -n true 2>/dev/null; then
@@ -304,7 +314,7 @@
     github_release_binary)
       # Check if we have curl or wget, and can write to ~/.local/bin
       if command -v curl >/dev/null 2>&1 || command -v wget >/dev/null 2>&1; then
         if [ -d "$HOME/.local/bin" ] || mkdir -p "$HOME/.local/bin" 2>/dev/null; then
           return 0
         fi
       fi
@@ -346,7 +356,7 @@
   echo "method=$method"
 
   # Get additional details based on method
   case "$method" in
     apt)
       if command -v dpkg >/dev/null 2>&1; then
         local pkg
@@ -392,7 +402,7 @@
       ;;
     github_release_binary)
       local version
       version="$("$binary" --version 2>/dev/null | head -1 || echo "unknown")"
       echo "version=$version"
       ;;
   esac
@@ -416,9 +426,9 @@
 # This checks both method availability AND tool-specific requirements
 # Args: tool_name, method, catalog_config (JSON string)
 can_install_via_method() {
   local tool="$1"
   local method="$2"
-  local config="${3:-{}}"
+  # $3 (catalog_config JSON) is accepted for future method-specific checks
 
   # First check if method is available
   if ! is_method_available "$method"; then

scripts/lib/reconcile.sh

@@ -18,19 +18,23 @@ RECONCILE_LIB_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 ensure_nvm_loaded
 
 # Remove an installation via a specific method
-# Args: tool_name, method, binary_name
+# Args: tool_name, method, binary_name, [binary_path]
+# binary_path: the detected installation's path. With multiple installations,
+# command -v resolves to whichever shadows the others in PATH — pass the
+# detected path so the right installation is removed.
 remove_installation() {
   local tool="$1"
   local method="$2"
   local binary="${3:-$tool}"
+  local known_path="${4:-}"
 
   echo "[$tool] Removing installation via $method..." >&2
 
   case "$method" in
     apt)
       # Find package name
       local binary_path
-      binary_path="$(command -v "$binary" 2>/dev/null || echo "")"
+      binary_path="${known_path:-$(command -v "$binary" 2>/dev/null || echo "")}"
       if [ -z "$binary_path" ]; then
         echo "[$tool] Binary not found, nothing to remove" >&2
         return 0
@@ -66,10 +70,16 @@ remove_installation() {
       fi
       ;;
     nvm)
-      # nvm-managed binaries: these are node versions, not directly removable via nvm here
-      # Skip removal — nvm versions are managed by install_node.sh
+      # nvm method is reserved for the node runtime itself (node/npm/npx/corepack);
+      # npm packages inside an nvm node dir classify as npm and are removed above.
       echo "[$tool] Skipping nvm-managed binary (use install_node.sh to manage)" >&2
       ;;
+    uv)
+      if command -v uv >/dev/null 2>&1; then
+        echo "[$tool] Uninstalling uv tool: $tool" >&2
+        uv tool uninstall "$tool" 2>/dev/null || true
+      fi
+      ;;
     gem)
       if command -v gem >/dev/null 2>&1; then
         echo "[$tool] Uninstalling gem: $tool" >&2
@@ -97,7 +107,7 @@ remove_installation() {
       ;;
     github_release_binary|manual)
       local binary_path
-      binary_path="$(command -v "$binary" 2>/dev/null || echo "")"
+      binary_path="${known_path:-$(command -v "$binary" 2>/dev/null || echo "")}"
       if [ -n "$binary_path" ] && [ -f "$binary_path" ]; then
         local bin_dir
         bin_dir="$(dirname "$binary_path")"

tests/test_uninstall_fixes.py

@@ -0,0 +1,270 @@
+"""Tests for uninstall fixes: uv removal, nvm npm-package removal,
+multi-install loop resilience, and apt path precision.
+
+Covers the three bugs found via `make uninstall-poetry` / `make uninstall-pnpm`:
+1. remove_installation had no `uv` handler, aborting the whole uninstall loop
+2. npm packages inside an nvm node dir classified as `nvm` and were skipped
+3. remove_installation re-resolved the binary via `command -v`, hitting the
+   wrong installation when multiple exist (apt removal silently no-oped)
+"""
+
+from __future__ import annotations
+
+import os
+import subprocess
+import sys
+import tempfile
+from pathlib import Path
+
+import pytest
+
+skip_on_windows = pytest.mark.skipif(
+    sys.platform == "win32", reason="Shell script tests require POSIX shell"
+)
+
+SCRIPTS_DIR = Path(__file__).parent.parent / "scripts"
+
+
+def _write_stub(stub_dir: Path, name: str, body: str) -> Path:
+    """Create an executable stub command in stub_dir."""
+    stub = stub_dir / name
+    stub.write_text(f"#!/usr/bin/env bash\n{body}\n")
+    stub.chmod(0o755)
+    return stub
+
+
+@skip_on_windows
+class TestRemoveInstallationUv:
+    """remove_installation must support the `uv` method (bug 1)."""
+
+    def _run(self, bash_code: str) -> subprocess.CompletedProcess:
+        full_code = f"""
+set -euo pipefail
+source "{SCRIPTS_DIR}/lib/reconcile.sh"
+{bash_code}
+"""
+        return subprocess.run(
+            ["bash", "-c", full_code],
+            capture_output=True, text=True, timeout=10,
+        )
+
+    def test_uv_method_calls_uv_tool_uninstall(self):
+        with tempfile.TemporaryDirectory() as tmpdir:
+            stub_dir = Path(tmpdir)
+            log_file = stub_dir / "calls.log"
+            _write_stub(stub_dir, "uv", f'echo "uv $*" >> "{log_file}"')
+
+            result = self._run(f"""
+export PATH="{stub_dir}:$PATH"
+remove_installation "poetry" "uv" "poetry"
+""")
+            assert result.returncode == 0, f"uv removal failed: {result.stderr}"
+            assert "tool uninstall poetry" in log_file.read_text()
+
+    def test_uv_method_with_failing_uv_does_not_crash(self):
+        result = self._run("""
+uv() { return 127; }
+remove_installation "poetry" "uv" "poetry" 2>&1 || true
+echo "SURVIVED"
+""")
+        assert "SURVIVED" in result.stdout
+
+
+@skip_on_windows
+class TestNvmClassification:
+    """npm packages inside an nvm node dir must classify as npm, not nvm (bug 2)."""
+
+    def _classify(self, tool: str, path: str) -> str:
+        full_code = f"""
+set -euo pipefail
+source "{SCRIPTS_DIR}/lib/capability.sh"
+classify_install_path "{tool}" "{path}"
+"""
+        result = subprocess.run(
+            ["bash", "-c", full_code],
+            capture_output=True, text=True, timeout=10,
+        )
+        assert result.returncode == 0, result.stderr
+        return result.stdout.strip()
+
+    def test_pnpm_in_nvm_dir_classifies_as_npm(self):
+        home = os.environ["HOME"]
+        assert (
+            self._classify("pnpm", f"{home}/.nvm/versions/node/v26.3.1/bin/pnpm")
+            == "npm(v26.3.1)"
+        )
+
+    def test_eslint_in_nvm_dir_classifies_as_npm(self):
+        home = os.environ["HOME"]
+        assert (
+            self._classify("eslint", f"{home}/.nvm/versions/node/v20.11.0/bin/eslint")
+            == "npm(v20.11.0)"
+        )
+
+    def test_node_runtime_binaries_stay_nvm(self):
+        home = os.environ["HOME"]
+        for runtime_bin in ("node", "npm", "npx", "corepack"):
+            assert (
+                self._classify(
+                    runtime_bin, f"{home}/.nvm/versions/node/v26.3.1/bin/{runtime_bin}"
+                )
+                == "nvm(v26.3.1)"
+            ), f"{runtime_bin} must stay nvm-managed"
+
+    def test_detect_install_method_npm_package_under_nvm(self):
+        """detect_install_method must make the same distinction as classify."""
+        home = os.environ["HOME"]
+        with tempfile.TemporaryDirectory() as tmpdir:
+            fake_nvm_bin = Path(f"{tmpdir}/home/.nvm/versions/node/v26.3.1/bin")
+            fake_nvm_bin.mkdir(parents=True)
+            for name in ("pnpm", "node"):
+                _write_stub(fake_nvm_bin, name, "echo fake")
+
+            full_code = f"""
+set -euo pipefail
+export HOME="{tmpdir}/home"
+source "{SCRIPTS_DIR}/lib/capability.sh"
+export PATH="{fake_nvm_bin}:$PATH"
+echo "pnpm=$(detect_install_method pnpm pnpm)"
+echo "node=$(detect_install_method node node)"
+"""
+            result = subprocess.run(
+                ["bash", "-c", full_code],
+                capture_output=True, text=True, timeout=10,
+            )
+            assert result.returncode == 0, result.stderr
+            assert "pnpm=npm" in result.stdout
+            assert "node=nvm" in result.stdout
+        # keep flake8 happy about unused import pattern parity
+        assert home
+
+
+@skip_on_windows
+class TestUninstallLoopResilience:
+    """One failing removal must not abort the multi-install loop (bug 1, class fix)."""
+
+    def test_unsupported_method_does_not_abort_loop(self):
+        """Replicate the install_tool.sh loop shape: a method the remover
+        rejects must not kill the pipeline under set -euo pipefail."""
+        full_code = f"""
+set -euo pipefail
+source "{SCRIPTS_DIR}/lib/reconcile.sh"
+printf 'weirdmethod:/tmp/nope\\nmanual:/tmp/alsonope\\n' | while IFS=: read -r method path; do
+  base_method="${{method%%(*}}"
+  remove_installation "sometool" "$base_method" "sometool" "$path" || true
+done
+echo "LOOP_COMPLETED"
+"""
+        result = subprocess.run(
+            ["bash", "-c", full_code],
+            capture_output=True, text=True, timeout=10,
+        )
+        assert result.returncode == 0, result.stderr
+        assert "LOOP_COMPLETED" in result.stdout
+
+    def test_install_tool_uninstall_loop_tolerates_removal_failure(self):
+        """The actual loop in install_tool.sh must guard remove_installation."""
+        content = (SCRIPTS_DIR / "install_tool.sh").read_text()
+        for line in content.splitlines():
+            stripped = line.strip()
+            if stripped.startswith("remove_installation "):
+                assert stripped.endswith("|| true"), (
+                    "remove_installation in the uninstall loop must not abort "
+                    f"the loop under set -e: {stripped!r}"
+                )
+                break
+        else:
+            pytest.fail("uninstall loop in install_tool.sh not found")
+
+
+@skip_on_windows
+class TestRemoveInstallationAptPathPrecision:
+    """The apt handler must use the detected path, not command -v (bug 3)."""
+
+    def _run_with_stubs(self, stub_dir: Path, bash_code: str) -> subprocess.CompletedProcess:
+        full_code = f"""
+set -euo pipefail
+source "{SCRIPTS_DIR}/lib/reconcile.sh"
+export PATH="{stub_dir}:$PATH"
+hash -r
+{bash_code}
+"""
+        return subprocess.run(
+            ["bash", "-c", full_code],
+            capture_output=True, text=True, timeout=10,
+        )
+
+    def _apt_stubs(self, stub_dir: Path, log_file: Path) -> None:
+        # dpkg knows only /usr/bin/poetry, owned by python3-poetry
+        _write_stub(stub_dir, "dpkg", f"""
+echo "dpkg $*" >> "{log_file}"
+case "$1" in
+  -S)
+    if [ "$2" = "/usr/bin/poetry" ]; then
+      echo "python3-poetry: /usr/bin/poetry"
+      exit 0
+    fi
+    exit 1
+    ;;
+  -s)
+    [ "$2" = "python3-poetry" ] && exit 0
+    exit 1
+    ;;
+esac
+exit 1
+""")
+        _write_stub(stub_dir, "sudo", f'echo "sudo $*" >> "{log_file}"')
+        _write_stub(stub_dir, "apt-get", f'echo "apt-get $*" >> "{log_file}"')
+
+    def test_apt_removal_uses_passed_path_over_command_v(self):
+        with tempfile.TemporaryDirectory() as tmpdir:
+            stub_dir = Path(tmpdir)
+            log_file = stub_dir / "calls.log"
+            log_file.touch()
+            self._apt_stubs(stub_dir, log_file)
+            # Decoy: command -v poetry resolves to this stub-dir binary,
+            # which dpkg does NOT know. Only the passed path works.
+            _write_stub(stub_dir, "poetry", "echo decoy")
+
+            result = self._run_with_stubs(stub_dir, """
+remove_installation "poetry" "apt" "poetry" "/usr/bin/poetry"
+""")
+            assert result.returncode == 0, result.stderr
+            log = log_file.read_text()
+            assert "dpkg -S /usr/bin/poetry" in log
+            assert "apt-get remove -y python3-poetry" in log
+
+    def test_apt_removal_falls_back_to_command_v_without_path(self):
+        with tempfile.TemporaryDirectory() as tmpdir:
+            stub_dir = Path(tmpdir)
+            log_file = stub_dir / "calls.log"
+            log_file.touch()
+            self._apt_stubs(stub_dir, log_file)
+
+            result = self._run_with_stubs(stub_dir, f"""
+cat > "{stub_dir}/poetry" <<'EOF'
+#!/usr/bin/env bash
+echo decoy
+EOF
+chmod +x "{stub_dir}/poetry"
+hash -r
+remove_installation "poetry" "apt" "poetry"
+""")
+            # Falls back to command -v; the decoy path is unknown to dpkg,
+            # so nothing is removed — but it must not crash.
+            assert result.returncode == 0, result.stderr
+
+    def test_manual_removal_uses_passed_path(self):
+        with tempfile.TemporaryDirectory() as tmpdir:
+            stub_dir = Path(tmpdir)
+            target_dir = Path(tmpdir) / "target"
+            target_dir.mkdir()
+            decoy = _write_stub(stub_dir, "sometool", "echo decoy")
+            target = _write_stub(target_dir, "sometool", "echo real")
+
+            result = self._run_with_stubs(stub_dir, f"""
+remove_installation "sometool" "manual" "sometool" "{target}"
+""")
+            assert result.returncode == 0, result.stderr
+            assert not target.exists(), "passed-path binary should be removed"
+            assert decoy.exists(), "PATH decoy must be untouched"