Skip to content

Commit cd3c011

Browse files
authored
fix(scripts): remove all detected installations on uninstall (#106)
## Summary Fixes three uninstall bugs found in a real `make uninstall-poetry` / `make uninstall-pnpm` session (poetry installed via both uv and apt; pnpm as an npm global inside nvm's node): 1. **No `uv` removal handler** — `remove_installation` fell through to `Unsupported removal method: uv` and its non-zero return aborted the whole removal loop under `set -e`, so the apt installation was never attempted. Added a `uv tool uninstall` handler and guarded the loop (`|| true`) — leftovers still surface via the post-removal verify. 2. **nvm misclassification** — npm packages inside an nvm node dir (pnpm, eslint, …) classified as `nvm(vX)` and were blanket-skipped. They now classify as `npm(vX)` and are removed via `npm uninstall -g`; only `node`/`npm`/`npx`/`corepack` stay nvm-managed. This matches the previously dormant assertion in `tests/test_guide_multi_install.sh` (`classify nvm path returns npm(...)`). 3. **Wrong-path resolution with multiple installs** — the apt and manual handlers re-resolved the binary via `command -v`, which hits whichever installation shadows the others in PATH; `dpkg -S` then missed and the apt removal silently no-oped. The detected path is now passed down and preferred. Also drops an unused `local config` in `can_install_via_method` — a pre-existing SC2034 that blocked the shellcheck pre-commit hook on any change to `capability.sh`. ## Tests - `tests/test_uninstall_fixes.py` (11 tests, TDD red→green): uv handler invocation, failing-uv resilience, nvm-vs-npm classification for both `classify_install_path` and `detect_install_method`, loop resilience, apt/manual path precision with dpkg/sudo/apt-get PATH stubs - Full suite: 673 passed, 1 skipped; `tests/test_guide_multi_install.sh`: 52 passed, 0 failed; shellcheck clean on the three touched scripts; smoke tests pass
2 parents cba0838 + 814daf9 commit cd3c011

4 files changed

Lines changed: 301 additions & 9 deletions

File tree

scripts/install_tool.sh

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,9 @@ if [ "$ACTION" = "uninstall" ]; then
9090
echo "[$TOOL] Skipping system binary: $path (managed by OS)" >&2
9191
continue
9292
fi
93-
remove_installation "$TOOL" "$base_method" "$binary_name"
93+
# || true: one failed removal (e.g. an unsupported method) must not abort
94+
# the remaining removals under set -e; leftovers surface in the verify below
95+
remove_installation "$TOOL" "$base_method" "$binary_name" "$path" || true
9496
done
9597

9698
# Verify removal (ignore system entries in the check)

scripts/lib/capability.sh

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,12 @@ detect_install_method() {
4949
return 0
5050
;;
5151
"$HOME/.nvm/"*)
52-
echo "nvm"
52+
# Only the node runtime itself is nvm-managed; anything else inside an
53+
# nvm node dir is an npm global package (removable via npm uninstall -g)
54+
case "$tool" in
55+
node|npm|npx|corepack) echo "nvm" ;;
56+
*) echo "npm" ;;
57+
esac
5358
return 0
5459
;;
5560
"/usr/local/bin/"*)
@@ -186,7 +191,12 @@ classify_install_path() {
186191
# Extract node version for context
187192
local node_version="${path#$HOME/.nvm/versions/node/}"
188193
node_version="${node_version%%/*}"
189-
echo "nvm($node_version)"
194+
# Only the node runtime itself is nvm-managed; anything else inside an
195+
# nvm node dir is an npm global package (removable via npm uninstall -g)
196+
case "$tool" in
197+
node|npm|npx|corepack) echo "nvm($node_version)" ;;
198+
*) echo "npm($node_version)" ;;
199+
esac
190200
;;
191201
"$HOME/.local/pipx/venvs/"*)
192202
local pkg="${path#$HOME/.local/pipx/venvs/}"
@@ -418,7 +428,7 @@ list_available_methods() {
418428
can_install_via_method() {
419429
local tool="$1"
420430
local method="$2"
421-
local config="${3:-{}}"
431+
# $3 (catalog_config JSON) is accepted for future method-specific checks
422432

423433
# First check if method is available
424434
if ! is_method_available "$method"; then

scripts/lib/reconcile.sh

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,19 +18,23 @@ RECONCILE_LIB_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
1818
ensure_nvm_loaded
1919

2020
# Remove an installation via a specific method
21-
# Args: tool_name, method, binary_name
21+
# Args: tool_name, method, binary_name, [binary_path]
22+
# binary_path: the detected installation's path. With multiple installations,
23+
# command -v resolves to whichever shadows the others in PATH — pass the
24+
# detected path so the right installation is removed.
2225
remove_installation() {
2326
local tool="$1"
2427
local method="$2"
2528
local binary="${3:-$tool}"
29+
local known_path="${4:-}"
2630

2731
echo "[$tool] Removing installation via $method..." >&2
2832

2933
case "$method" in
3034
apt)
3135
# Find package name
3236
local binary_path
33-
binary_path="$(command -v "$binary" 2>/dev/null || echo "")"
37+
binary_path="${known_path:-$(command -v "$binary" 2>/dev/null || echo "")}"
3438
if [ -z "$binary_path" ]; then
3539
echo "[$tool] Binary not found, nothing to remove" >&2
3640
return 0
@@ -66,10 +70,16 @@ remove_installation() {
6670
fi
6771
;;
6872
nvm)
69-
# nvm-managed binaries: these are node versions, not directly removable via nvm here
70-
# Skip removal — nvm versions are managed by install_node.sh
73+
# nvm method is reserved for the node runtime itself (node/npm/npx/corepack);
74+
# npm packages inside an nvm node dir classify as npm and are removed above.
7175
echo "[$tool] Skipping nvm-managed binary (use install_node.sh to manage)" >&2
7276
;;
77+
uv)
78+
if command -v uv >/dev/null 2>&1; then
79+
echo "[$tool] Uninstalling uv tool: $tool" >&2
80+
uv tool uninstall "$tool" 2>/dev/null || true
81+
fi
82+
;;
7383
gem)
7484
if command -v gem >/dev/null 2>&1; then
7585
echo "[$tool] Uninstalling gem: $tool" >&2
@@ -97,7 +107,7 @@ remove_installation() {
97107
;;
98108
github_release_binary|manual)
99109
local binary_path
100-
binary_path="$(command -v "$binary" 2>/dev/null || echo "")"
110+
binary_path="${known_path:-$(command -v "$binary" 2>/dev/null || echo "")}"
101111
if [ -n "$binary_path" ] && [ -f "$binary_path" ]; then
102112
local bin_dir
103113
bin_dir="$(dirname "$binary_path")"

tests/test_uninstall_fixes.py

Lines changed: 270 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,270 @@
1+
"""Tests for uninstall fixes: uv removal, nvm npm-package removal,
2+
multi-install loop resilience, and apt path precision.
3+
4+
Covers the three bugs found via `make uninstall-poetry` / `make uninstall-pnpm`:
5+
1. remove_installation had no `uv` handler, aborting the whole uninstall loop
6+
2. npm packages inside an nvm node dir classified as `nvm` and were skipped
7+
3. remove_installation re-resolved the binary via `command -v`, hitting the
8+
wrong installation when multiple exist (apt removal silently no-oped)
9+
"""
10+
11+
from __future__ import annotations
12+
13+
import os
14+
import subprocess
15+
import sys
16+
import tempfile
17+
from pathlib import Path
18+
19+
import pytest
20+
21+
skip_on_windows = pytest.mark.skipif(
22+
sys.platform == "win32", reason="Shell script tests require POSIX shell"
23+
)
24+
25+
SCRIPTS_DIR = Path(__file__).parent.parent / "scripts"
26+
27+
28+
def _write_stub(stub_dir: Path, name: str, body: str) -> Path:
29+
"""Create an executable stub command in stub_dir."""
30+
stub = stub_dir / name
31+
stub.write_text(f"#!/usr/bin/env bash\n{body}\n")
32+
stub.chmod(0o755)
33+
return stub
34+
35+
36+
@skip_on_windows
37+
class TestRemoveInstallationUv:
38+
"""remove_installation must support the `uv` method (bug 1)."""
39+
40+
def _run(self, bash_code: str) -> subprocess.CompletedProcess:
41+
full_code = f"""
42+
set -euo pipefail
43+
source "{SCRIPTS_DIR}/lib/reconcile.sh"
44+
{bash_code}
45+
"""
46+
return subprocess.run(
47+
["bash", "-c", full_code],
48+
capture_output=True, text=True, timeout=10,
49+
)
50+
51+
def test_uv_method_calls_uv_tool_uninstall(self):
52+
with tempfile.TemporaryDirectory() as tmpdir:
53+
stub_dir = Path(tmpdir)
54+
log_file = stub_dir / "calls.log"
55+
_write_stub(stub_dir, "uv", f'echo "uv $*" >> "{log_file}"')
56+
57+
result = self._run(f"""
58+
export PATH="{stub_dir}:$PATH"
59+
remove_installation "poetry" "uv" "poetry"
60+
""")
61+
assert result.returncode == 0, f"uv removal failed: {result.stderr}"
62+
assert "tool uninstall poetry" in log_file.read_text()
63+
64+
def test_uv_method_with_failing_uv_does_not_crash(self):
65+
result = self._run("""
66+
uv() { return 127; }
67+
remove_installation "poetry" "uv" "poetry" 2>&1 || true
68+
echo "SURVIVED"
69+
""")
70+
assert "SURVIVED" in result.stdout
71+
72+
73+
@skip_on_windows
74+
class TestNvmClassification:
75+
"""npm packages inside an nvm node dir must classify as npm, not nvm (bug 2)."""
76+
77+
def _classify(self, tool: str, path: str) -> str:
78+
full_code = f"""
79+
set -euo pipefail
80+
source "{SCRIPTS_DIR}/lib/capability.sh"
81+
classify_install_path "{tool}" "{path}"
82+
"""
83+
result = subprocess.run(
84+
["bash", "-c", full_code],
85+
capture_output=True, text=True, timeout=10,
86+
)
87+
assert result.returncode == 0, result.stderr
88+
return result.stdout.strip()
89+
90+
def test_pnpm_in_nvm_dir_classifies_as_npm(self):
91+
home = os.environ["HOME"]
92+
assert (
93+
self._classify("pnpm", f"{home}/.nvm/versions/node/v26.3.1/bin/pnpm")
94+
== "npm(v26.3.1)"
95+
)
96+
97+
def test_eslint_in_nvm_dir_classifies_as_npm(self):
98+
home = os.environ["HOME"]
99+
assert (
100+
self._classify("eslint", f"{home}/.nvm/versions/node/v20.11.0/bin/eslint")
101+
== "npm(v20.11.0)"
102+
)
103+
104+
def test_node_runtime_binaries_stay_nvm(self):
105+
home = os.environ["HOME"]
106+
for runtime_bin in ("node", "npm", "npx", "corepack"):
107+
assert (
108+
self._classify(
109+
runtime_bin, f"{home}/.nvm/versions/node/v26.3.1/bin/{runtime_bin}"
110+
)
111+
== "nvm(v26.3.1)"
112+
), f"{runtime_bin} must stay nvm-managed"
113+
114+
def test_detect_install_method_npm_package_under_nvm(self):
115+
"""detect_install_method must make the same distinction as classify."""
116+
home = os.environ["HOME"]
117+
with tempfile.TemporaryDirectory() as tmpdir:
118+
fake_nvm_bin = Path(f"{tmpdir}/home/.nvm/versions/node/v26.3.1/bin")
119+
fake_nvm_bin.mkdir(parents=True)
120+
for name in ("pnpm", "node"):
121+
_write_stub(fake_nvm_bin, name, "echo fake")
122+
123+
full_code = f"""
124+
set -euo pipefail
125+
export HOME="{tmpdir}/home"
126+
source "{SCRIPTS_DIR}/lib/capability.sh"
127+
export PATH="{fake_nvm_bin}:$PATH"
128+
echo "pnpm=$(detect_install_method pnpm pnpm)"
129+
echo "node=$(detect_install_method node node)"
130+
"""
131+
result = subprocess.run(
132+
["bash", "-c", full_code],
133+
capture_output=True, text=True, timeout=10,
134+
)
135+
assert result.returncode == 0, result.stderr
136+
assert "pnpm=npm" in result.stdout
137+
assert "node=nvm" in result.stdout
138+
# keep flake8 happy about unused import pattern parity
139+
assert home
140+
141+
142+
@skip_on_windows
143+
class TestUninstallLoopResilience:
144+
"""One failing removal must not abort the multi-install loop (bug 1, class fix)."""
145+
146+
def test_unsupported_method_does_not_abort_loop(self):
147+
"""Replicate the install_tool.sh loop shape: a method the remover
148+
rejects must not kill the pipeline under set -euo pipefail."""
149+
full_code = f"""
150+
set -euo pipefail
151+
source "{SCRIPTS_DIR}/lib/reconcile.sh"
152+
printf 'weirdmethod:/tmp/nope\\nmanual:/tmp/alsonope\\n' | while IFS=: read -r method path; do
153+
base_method="${{method%%(*}}"
154+
remove_installation "sometool" "$base_method" "sometool" "$path" || true
155+
done
156+
echo "LOOP_COMPLETED"
157+
"""
158+
result = subprocess.run(
159+
["bash", "-c", full_code],
160+
capture_output=True, text=True, timeout=10,
161+
)
162+
assert result.returncode == 0, result.stderr
163+
assert "LOOP_COMPLETED" in result.stdout
164+
165+
def test_install_tool_uninstall_loop_tolerates_removal_failure(self):
166+
"""The actual loop in install_tool.sh must guard remove_installation."""
167+
content = (SCRIPTS_DIR / "install_tool.sh").read_text()
168+
for line in content.splitlines():
169+
stripped = line.strip()
170+
if stripped.startswith("remove_installation "):
171+
assert stripped.endswith("|| true"), (
172+
"remove_installation in the uninstall loop must not abort "
173+
f"the loop under set -e: {stripped!r}"
174+
)
175+
break
176+
else:
177+
pytest.fail("uninstall loop in install_tool.sh not found")
178+
179+
180+
@skip_on_windows
181+
class TestRemoveInstallationAptPathPrecision:
182+
"""The apt handler must use the detected path, not command -v (bug 3)."""
183+
184+
def _run_with_stubs(self, stub_dir: Path, bash_code: str) -> subprocess.CompletedProcess:
185+
full_code = f"""
186+
set -euo pipefail
187+
source "{SCRIPTS_DIR}/lib/reconcile.sh"
188+
export PATH="{stub_dir}:$PATH"
189+
hash -r
190+
{bash_code}
191+
"""
192+
return subprocess.run(
193+
["bash", "-c", full_code],
194+
capture_output=True, text=True, timeout=10,
195+
)
196+
197+
def _apt_stubs(self, stub_dir: Path, log_file: Path) -> None:
198+
# dpkg knows only /usr/bin/poetry, owned by python3-poetry
199+
_write_stub(stub_dir, "dpkg", f"""
200+
echo "dpkg $*" >> "{log_file}"
201+
case "$1" in
202+
-S)
203+
if [ "$2" = "/usr/bin/poetry" ]; then
204+
echo "python3-poetry: /usr/bin/poetry"
205+
exit 0
206+
fi
207+
exit 1
208+
;;
209+
-s)
210+
[ "$2" = "python3-poetry" ] && exit 0
211+
exit 1
212+
;;
213+
esac
214+
exit 1
215+
""")
216+
_write_stub(stub_dir, "sudo", f'echo "sudo $*" >> "{log_file}"')
217+
_write_stub(stub_dir, "apt-get", f'echo "apt-get $*" >> "{log_file}"')
218+
219+
def test_apt_removal_uses_passed_path_over_command_v(self):
220+
with tempfile.TemporaryDirectory() as tmpdir:
221+
stub_dir = Path(tmpdir)
222+
log_file = stub_dir / "calls.log"
223+
log_file.touch()
224+
self._apt_stubs(stub_dir, log_file)
225+
# Decoy: command -v poetry resolves to this stub-dir binary,
226+
# which dpkg does NOT know. Only the passed path works.
227+
_write_stub(stub_dir, "poetry", "echo decoy")
228+
229+
result = self._run_with_stubs(stub_dir, """
230+
remove_installation "poetry" "apt" "poetry" "/usr/bin/poetry"
231+
""")
232+
assert result.returncode == 0, result.stderr
233+
log = log_file.read_text()
234+
assert "dpkg -S /usr/bin/poetry" in log
235+
assert "apt-get remove -y python3-poetry" in log
236+
237+
def test_apt_removal_falls_back_to_command_v_without_path(self):
238+
with tempfile.TemporaryDirectory() as tmpdir:
239+
stub_dir = Path(tmpdir)
240+
log_file = stub_dir / "calls.log"
241+
log_file.touch()
242+
self._apt_stubs(stub_dir, log_file)
243+
244+
result = self._run_with_stubs(stub_dir, f"""
245+
cat > "{stub_dir}/poetry" <<'EOF'
246+
#!/usr/bin/env bash
247+
echo decoy
248+
EOF
249+
chmod +x "{stub_dir}/poetry"
250+
hash -r
251+
remove_installation "poetry" "apt" "poetry"
252+
""")
253+
# Falls back to command -v; the decoy path is unknown to dpkg,
254+
# so nothing is removed — but it must not crash.
255+
assert result.returncode == 0, result.stderr
256+
257+
def test_manual_removal_uses_passed_path(self):
258+
with tempfile.TemporaryDirectory() as tmpdir:
259+
stub_dir = Path(tmpdir)
260+
target_dir = Path(tmpdir) / "target"
261+
target_dir.mkdir()
262+
decoy = _write_stub(stub_dir, "sometool", "echo decoy")
263+
target = _write_stub(target_dir, "sometool", "echo real")
264+
265+
result = self._run_with_stubs(stub_dir, f"""
266+
remove_installation "sometool" "manual" "sometool" "{target}"
267+
""")
268+
assert result.returncode == 0, result.stderr
269+
assert not target.exists(), "passed-path binary should be removed"
270+
assert decoy.exists(), "PATH decoy must be untouched"

0 commit comments

Comments
 (0)