From 97a938f990ebeea2b2cff50d8b5de15e3b5aea58 Mon Sep 17 00:00:00 2001 From: Hassan Zidan Date: Sun, 11 Oct 2026 14:53:10 +0300 Subject: [PATCH] fix(install): never let uninstall --purge delete the project when GRAPHIFY_OUT is empty or "." A set-but-empty GRAPHIFY_OUT (a blank .env / docker-compose / CI entry) or GRAPHIFY_OUT=. made the purge target /"" == the project itself, and rmtree deleted the whole project. - paths: treat a blank GRAPHIFY_OUT as unset; "." / ".." fall back to graphify-out with a warning. GRAPHIFY_OUT_NAME and out_path() follow. - install: before any uninstaller runs, refuse --purge (exit 1, nothing changed) when the target is the project, a parent of it, the home folder or a filesystem root. This also covers an absolute GRAPHIFY_OUT. - hooks: the rebuild bodies resolve the output dir via graphify.paths instead of reading the env var directly. Fixes #4328 Co-Authored-By: Claude Opus 5.5 (cherry picked from commit 7d6befbb9c027f1c01070c77358f37cbb1b9ff88) --- graphify/hooks.py | 4 +- graphify/install.py | 33 +++- graphify/paths.py | 27 +++- tests/test_hooks.py | 29 ++++ tests/test_uninstall_purge_safety.py | 215 +++++++++++++++++++++++++++ 5 files changed, 304 insertions(+), 4 deletions(-) create mode 100644 tests/test_uninstall_purge_safety.py diff --git a/graphify/hooks.py b/graphify/hooks.py index 9b52dd8d4..3da55a622 100644 --- a/graphify/hooks.py +++ b/graphify/hooks.py @@ -221,7 +221,7 @@ try: _watchdog.start() _force = os.environ.get('GRAPHIFY_FORCE', '').lower() in ('1', 'true', 'yes') _root = Path('.') - _out = os.environ.get('GRAPHIFY_OUT', 'graphify-out') + from graphify.paths import GRAPHIFY_OUT as _out _saved = Path(_out) / '.graphify_root' if _saved.exists(): _txt = _saved.read_text(encoding='utf-8-sig').strip() @@ -296,7 +296,7 @@ try: # (no changed_paths) is correct here. The flock inside _rebuild_code still # prevents pile-ups when commit + checkout fire back-to-back. _root = Path('.') - _out = os.environ.get('GRAPHIFY_OUT', 'graphify-out') + from graphify.paths import GRAPHIFY_OUT as _out _saved = Path(_out) / '.graphify_root' if _saved.exists(): _txt = _saved.read_text(encoding='utf-8-sig').strip() diff --git a/graphify/install.py b/graphify/install.py index 921463283..85c690ac5 100644 --- a/graphify/install.py +++ b/graphify/install.py @@ -2105,9 +2105,41 @@ def _strip_graphify_hook(settings_path: Path) -> None: settings["hooks"]["PreToolUse"] = filtered settings_path.write_text(json.dumps(settings, indent=2), encoding="utf-8") print(f" .claude/{settings_path.name} -> PreToolUse hook removed") + + +def _unsafe_purge_reason(out: Path, project_dir: Path) -> str | None: + """Why ``rmtree(out)`` must not run for ``--purge``, or None when it is safe. + + The output dir may legitimately live outside the project (an absolute + ``GRAPHIFY_OUT``), so this only rejects targets whose deletion would take + the project, the home directory, or a filesystem root with it. + """ + target = out.resolve() + project = project_dir.resolve() + home = Path.home().resolve() + if target == project or target in project.parents: + return "the project folder or one of its parents" + if target == home or target in home.parents: + return "your home folder or one of its parents" + if target == Path(target.anchor): + return "a filesystem root" + return None + + def uninstall_all(project_dir: Path | None = None, purge: bool = False) -> None: """Remove graphify from every platform detected in the current project.""" pd = project_dir or Path(".") + out = pd / _GRAPHIFY_OUT + if purge: + # Checked before any uninstaller runs, so a refused purge changes nothing. + reason = _unsafe_purge_reason(out, pd) + if reason: + print( + f"error: refusing --purge: {out.resolve()} is {reason}. " + "Nothing was changed. Check the GRAPHIFY_OUT environment variable.", + file=sys.stderr, + ) + sys.exit(1) print("Uninstalling graphify from all detected platforms...\n") # Skill-file / config-section uninstallers. remove_user_skill=True keeps the @@ -2142,7 +2174,6 @@ def uninstall_all(project_dir: Path | None = None, purge: bool = False) -> None: if purge: import shutil as _shutil - out = pd / _GRAPHIFY_OUT if out.exists(): _shutil.rmtree(out) print(f"\n {_GRAPHIFY_OUT}/ -> deleted (--purge)") diff --git a/graphify/paths.py b/graphify/paths.py index 0cf500bed..fb828cbfb 100644 --- a/graphify/paths.py +++ b/graphify/paths.py @@ -22,10 +22,35 @@ import json import os import re import stat +import sys import tempfile from pathlib import Path, PurePosixPath, PureWindowsPath -GRAPHIFY_OUT = os.environ.get("GRAPHIFY_OUT", "graphify-out") + +def _out_from_env() -> str: + """``GRAPHIFY_OUT``, or ``"graphify-out"`` when it is unset, empty, or names + the project itself / a parent of it. + + ``os.environ.get(key, default)`` only applies the default when the variable + is missing, so ``GRAPHIFY_OUT=`` (a blank ``.env`` entry) used to yield + ``""``: output landed in the project root and ``uninstall --purge`` deleted + the whole project. ``.`` and ``..`` are never a valid output dir either. + """ + value = (os.environ.get("GRAPHIFY_OUT") or "").strip() + if not value: + return "graphify-out" + norm = os.path.normpath(value) + if norm == "." or all(part == ".." for part in Path(norm).parts): + print( + f"warning: GRAPHIFY_OUT={value!r} points at the project or a parent " + "of it; using 'graphify-out' instead", + file=sys.stderr, + ) + return "graphify-out" + return value + + +GRAPHIFY_OUT = _out_from_env() def os_replace_with_fallback(src: "str | Path", dst: "str | Path") -> None: diff --git a/tests/test_hooks.py b/tests/test_hooks.py index 6c208c70b..1df3fcce4 100644 --- a/tests/test_hooks.py +++ b/tests/test_hooks.py @@ -384,6 +384,35 @@ def test_rebuild_bodies_honour_an_in_repo_graphify_root(name, body, tmp_path, mo assert ns["_root"].resolve() == (repo / "backend").resolve(), f"{name} lost the scoped root" +@pytest.mark.parametrize( + "name,body", + [("post-commit", _REBUILD_BODY_COMMIT), ("post-checkout", _REBUILD_BODY_CHECKOUT)], +) +def test_rebuild_bodies_treat_blank_graphify_out_as_default(name, body, tmp_path): + """A set-but-empty GRAPHIFY_OUT must resolve to graphify-out/ like every other + reader (paths._out_from_env), not to the repo root, or the hook misses + .graphify_root. Runs in a fresh process because the hook reads the value + when graphify.paths is first imported.""" + repo = tmp_path / "repo" + (repo / "graphify-out").mkdir(parents=True) + (repo / "backend").mkdir() + (repo / "graphify-out" / ".graphify_root").write_text("backend", encoding="utf-8") + script = ( + "import os\nfrom pathlib import Path\n" + + _extract_root_resolution(body) + + "\nprint(_root.resolve())\n" + ) + result = subprocess.run( + [sys.executable, "-c", script], + cwd=repo, + env={**os.environ, "GRAPHIFY_OUT": ""}, + capture_output=True, + text=True, + check=True, + ) + assert Path(result.stdout.strip()) == (repo / "backend").resolve(), f"{name} ignored graphify-out/" + + @pytest.mark.skipif(sys.platform == "win32", reason="symlink setup differs on Windows") @pytest.mark.parametrize( "name,body", diff --git a/tests/test_uninstall_purge_safety.py b/tests/test_uninstall_purge_safety.py new file mode 100644 index 000000000..9d0ed9921 --- /dev/null +++ b/tests/test_uninstall_purge_safety.py @@ -0,0 +1,215 @@ +"""`graphify uninstall --purge` must only ever delete the output directory. + +Regression: a set-but-empty ``GRAPHIFY_OUT`` (``GRAPHIFY_OUT=`` in a ``.env``, +docker-compose or CI config) or ``GRAPHIFY_OUT=.`` made the purge target +``/""`` == the project itself, and ``rmtree`` deleted the whole project. +Two independent layers guard against it: ``paths._out_from_env`` never yields +the project (or a parent) as the output dir, and ``uninstall_all`` refuses to +purge a target that would take the project, home or a filesystem root with it, +even when an absolute ``GRAPHIFY_OUT`` bypasses the first layer. +""" +from __future__ import annotations + +import sys +from pathlib import Path + +import pytest + +import graphify.hooks as hooks_mod +import graphify.install as install_mod +from graphify import paths + +_PLATFORM_UNINSTALLERS = [ + "claude_uninstall", + "codebuddy_uninstall", + "gemini_uninstall", + "vscode_uninstall", + "_cursor_uninstall", + "_kiro_uninstall", + "_antigravity_uninstall", + "_agents_uninstall", + "_remove_skill_file", + "_uninstall_opencode_plugin", + "_uninstall_codex_hook", +] + + +@pytest.fixture +def env(tmp_path, monkeypatch, _sandbox_home): + """A throwaway project and every platform uninstaller stubbed so only the + purge step can touch the filesystem. HOME / USERPROFILE / Path.home() already + point at the conftest sandbox home. Returns the stub call log.""" + home = _sandbox_home + monkeypatch.delenv("GRAPHIFY_OUT", raising=False) + + project = tmp_path / "parent" / "project" + (project / "src").mkdir(parents=True) + (project / "src" / "important.py").write_text("x = 1\n", encoding="utf-8") + (project / "graphify-out").mkdir() + (project / "graphify-out" / "graph.json").write_text("{}", encoding="utf-8") + + calls: list[str] = [] + for name in _PLATFORM_UNINSTALLERS: + monkeypatch.setattr(install_mod, name, lambda *a, _n=name, **k: calls.append(_n)) + monkeypatch.setattr(hooks_mod, "uninstall", lambda *a, **k: calls.append("hooks.uninstall")) + # Run from outside the project: Windows cannot rmtree the current directory. + monkeypatch.chdir(tmp_path) + return {"project": project, "home": home, "calls": calls, "tmp": tmp_path} + + +def _purge_with_env(monkeypatch, project, value): + """Run the real purge with GRAPHIFY_OUT resolved exactly as paths.py does at + import time (no module reload, so no state leaks into other tests).""" + monkeypatch.setenv("GRAPHIFY_OUT", value) + monkeypatch.setattr(install_mod, "_GRAPHIFY_OUT", paths._out_from_env()) + install_mod.uninstall_all(project_dir=project, purge=True) + + +def _assert_project_intact(project): + assert project.is_dir() + assert (project / "src" / "important.py").is_file() + + +# -- Layer 1: paths._out_from_env ----------------------------------------- + + +@pytest.mark.parametrize("value", ["", " ", "\t"]) +def test_blank_graphify_out_falls_back_to_default(monkeypatch, value): + monkeypatch.setenv("GRAPHIFY_OUT", value) + assert paths._out_from_env() == "graphify-out" + + +@pytest.mark.parametrize("value", [".", "./", "..", "../..", " . "]) +def test_project_or_parent_graphify_out_falls_back_with_warning(monkeypatch, capsys, value): + monkeypatch.setenv("GRAPHIFY_OUT", value) + assert paths._out_from_env() == "graphify-out" + assert "warning: GRAPHIFY_OUT=" in capsys.readouterr().err + + +@pytest.mark.parametrize("value", ["custom-out", "../shared-out", "out/graphify"]) +def test_valid_relative_graphify_out_is_kept(monkeypatch, capsys, value): + monkeypatch.setenv("GRAPHIFY_OUT", value) + assert paths._out_from_env() == value + assert capsys.readouterr().err == "" + + +def test_graphify_out_is_stripped(monkeypatch): + monkeypatch.setenv("GRAPHIFY_OUT", " custom-out ") + assert paths._out_from_env() == "custom-out" + + +def test_absolute_graphify_out_is_kept(monkeypatch, tmp_path): + monkeypatch.setenv("GRAPHIFY_OUT", str(tmp_path / "shared-out")) + assert paths._out_from_env() == str(tmp_path / "shared-out") + + +def test_unset_graphify_out_is_default(monkeypatch): + monkeypatch.delenv("GRAPHIFY_OUT", raising=False) + assert paths._out_from_env() == "graphify-out" + + +# -- End to end: the purge never deletes the project ------------------------ + + +@pytest.mark.parametrize("value", ["", " ", ".", ".."]) +def test_purge_with_bad_graphify_out_only_removes_default_out(env, monkeypatch, value): + project = env["project"] + _purge_with_env(monkeypatch, project, value) + _assert_project_intact(project) + assert not (project / "graphify-out").exists() + # ".." must not have reached the parent of the project either. + assert project.parent.is_dir() + + +def test_purge_default_removes_only_graphify_out(env): + project = env["project"] + install_mod.uninstall_all(project_dir=project, purge=True) + _assert_project_intact(project) + assert not (project / "graphify-out").exists() + + +def test_purge_custom_relative_out(env, monkeypatch): + project = env["project"] + (project / "custom-out").mkdir() + (project / "custom-out" / "graph.json").write_text("{}", encoding="utf-8") + _purge_with_env(monkeypatch, project, "custom-out") + _assert_project_intact(project) + assert not (project / "custom-out").exists() + assert (project / "graphify-out").exists() # not the configured dir + + +def test_purge_custom_absolute_out_outside_project(env, monkeypatch): + """A shared output dir outside the project (#686, #1423) stays purgeable.""" + project = env["project"] + shared = env["tmp"] / "shared-out" + shared.mkdir() + (shared / "graph.json").write_text("{}", encoding="utf-8") + _purge_with_env(monkeypatch, project, str(shared)) + _assert_project_intact(project) + assert not shared.exists() + + +# -- Layer 2: unsafe targets are refused before anything changes ------------ + + +def _assert_refused(env, capsys, excinfo): + assert excinfo.value.code == 1 + assert "refusing --purge" in capsys.readouterr().err + _assert_project_intact(env["project"]) + assert (env["project"] / "graphify-out" / "graph.json").is_file() + assert env["calls"] == [], "a refused purge must not run any uninstaller" + + +def test_purge_refuses_absolute_out_equal_to_project(env, monkeypatch, capsys): + with pytest.raises(SystemExit) as excinfo: + _purge_with_env(monkeypatch, env["project"], str(env["project"])) + _assert_refused(env, capsys, excinfo) + + +def test_purge_refuses_absolute_out_parent_of_project(env, monkeypatch, capsys): + with pytest.raises(SystemExit) as excinfo: + _purge_with_env(monkeypatch, env["project"], str(env["project"].parent)) + _assert_refused(env, capsys, excinfo) + assert env["project"].parent.is_dir() + + +def test_purge_refuses_home(env, monkeypatch, capsys): + (env["home"] / "keep.txt").write_text("x", encoding="utf-8") + with pytest.raises(SystemExit) as excinfo: + _purge_with_env(monkeypatch, env["project"], str(env["home"])) + _assert_refused(env, capsys, excinfo) + assert (env["home"] / "keep.txt").is_file() + + +def test_unsafe_purge_reason_rejects_filesystem_roots(env): + """Checked on the pure helper only: a regression here must never be able to + hand a real filesystem root to rmtree.""" + project = env["project"] + root = Path(project.resolve().anchor) + assert install_mod._unsafe_purge_reason(root, project) is not None + if sys.platform == "win32": + # A root on another drive is not a parent of the project, but still refused. + assert install_mod._unsafe_purge_reason(Path("Z:\\"), project) == "a filesystem root" + + +def test_unsafe_purge_reason_allows_out_dirs(env): + project = env["project"] + assert install_mod._unsafe_purge_reason(project / "graphify-out", project) is None + assert install_mod._unsafe_purge_reason(env["tmp"] / "shared-out", project) is None + + +@pytest.mark.parametrize("value", ["", "."]) +def test_purge_refuses_project_when_layer_one_is_bypassed(env, monkeypatch, capsys, value): + """The safety net must hold on its own, even if a bad value reaches install.""" + monkeypatch.setattr(install_mod, "_GRAPHIFY_OUT", value) + with pytest.raises(SystemExit) as excinfo: + install_mod.uninstall_all(project_dir=env["project"], purge=True) + _assert_refused(env, capsys, excinfo) + + +def test_uninstall_without_purge_ignores_unsafe_out(env, monkeypatch): + """The safety check gates only the purge; plain uninstall is unchanged.""" + monkeypatch.setattr(install_mod, "_GRAPHIFY_OUT", "") + install_mod.uninstall_all(project_dir=env["project"], purge=False) + _assert_project_intact(env["project"]) + assert "claude_uninstall" in env["calls"]