diff --git a/src/specify_cli/integrations/_file_changes.py b/src/specify_cli/integrations/_file_changes.py index 9ed0ebc62f..dd9878bee3 100644 --- a/src/specify_cli/integrations/_file_changes.py +++ b/src/specify_cli/integrations/_file_changes.py @@ -45,3 +45,10 @@ def write_bytes(path: Path, content: bytes) -> int: def unlink(path: Path, *, missing_ok: bool = False) -> None: with changing_file(path, removal=True): path.unlink(missing_ok=missing_ok) + + +def rmdir(path: Path) -> None: + """Journal and remove an empty directory without snapshotting user files.""" + if not any(path.iterdir()): + with changing_file(path, removal=True): + path.rmdir() diff --git a/src/specify_cli/presets/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index 5a4a4fb739..18baf05b4b 100644 --- a/src/specify_cli/presets/_manager_skills.py +++ b/src/specify_cli/presets/_manager_skills.py @@ -2,7 +2,6 @@ import copy import os -import shutil from pathlib import Path from typing import TYPE_CHECKING, Any, Dict, List, Optional, Set, Union @@ -216,7 +215,7 @@ def apply_to_dir( managed_skill_names if managed_names is None else managed_names ) # Restore skills for commands whose winner is non-preset. - # _unregister_skills_in_dir can rmtree the skill dir, so + # _unregister_skills_in_dir can remove the skill file, so # overrides must be handled directly (create dir + write) # without that call. dir_core_ext_names = [ @@ -1275,11 +1274,13 @@ def _delete_agent_preset_skills( if override_source: owned_sources.add(override_source) if source in owned_sources: - from ..integrations._file_changes import after_file_change, before_file_change + from ..integrations._file_changes import rmdir, unlink - before_file_change(skill_subdir, removal=True) - shutil.rmtree(skill_subdir) - after_file_change(skill_subdir) + unlink(skill_file) + try: + rmdir(skill_subdir) + except OSError: + pass @staticmethod def _warn_unrestored_skill( @@ -1370,7 +1371,7 @@ def _unregister_skills_in_dir( # is_dir() follows symlinks, so a symlinked skill subdirectory # (with a safe, non-symlinked parent) would otherwise slip past # _safe_skills_dir_for_agent's parent-only check and have - # write_text/rmtree operate through it (#2948). + # writes/removals operate through it (#2948). if not self._validate_skill_subdir( skill_subdir, create=False, skills_root=skills_dir ): @@ -1435,7 +1436,7 @@ def _unregister_skills_in_dir( # source cannot produce restored content, so leave the # existing skill untouched rather than leaking a raw # OSError/UnicodeDecodeError out of `preset remove` — and - # rather than falling through to the rmtree below, which + # rather than falling through to the removal below, which # would delete a skill precisely when its replacement # cannot be generated. Matches the `continue` guards above # (unsafe name, missing subdir, foreign owner), which also @@ -1541,12 +1542,14 @@ def _unregister_skills_in_dir( _write_shared_text(skills_dir, skill_file, skill_content) mutated_names.append(skill_name) else: - # No core or extension template — remove the skill entirely - from ..integrations._file_changes import after_file_change, before_file_change + # No core or extension template — remove only the generated file + from ..integrations._file_changes import rmdir, unlink - before_file_change(skill_subdir, removal=True) - shutil.rmtree(skill_subdir) - after_file_change(skill_subdir) + unlink(skill_file) + try: + rmdir(skill_subdir) + except OSError: + pass mutated_names.append(skill_name) return mutated_names diff --git a/tests/specify_cli/presets/test_command_remove.py b/tests/specify_cli/presets/test_command_remove.py index 50a7e89f7b..dddb395248 100644 --- a/tests/specify_cli/presets/test_command_remove.py +++ b/tests/specify_cli/presets/test_command_remove.py @@ -2,9 +2,12 @@ from __future__ import annotations +import os +import stat from pathlib import Path from unittest.mock import patch +import pytest from typer.testing import CliRunner from specify_cli import app @@ -19,3 +22,101 @@ def test_remove_installed_preset(project_dir, pack_dir): assert result.exit_code == 0, result.output assert not PresetManager(project_dir).registry.is_installed("test-pack") + + +@pytest.mark.parametrize("operation", ["remove", "switch"]) +@pytest.mark.parametrize("user_reference", [False, True]) +def test_preset_skill_cleanup_preserves_user_files(tmp_path, monkeypatch, operation, user_reference): + monkeypatch.chdir(tmp_path) + runner = CliRunner() + preset = Path(__file__).resolve().parents[3] / "presets" / "scaffold" + for args in ( + ["init", "--here", "--integration", "copilot", "--script", "sh", "--ignore-agent-tools"], + ["preset", "add", "--dev", str(preset)], + ): + result = runner.invoke(app, args, catch_exceptions=False) + assert result.exit_code == 0, result.output + skill_dir = tmp_path / ".github/skills/speckit-myext-myextcmd" + reference = skill_dir / "references" / "user.md" + original = b"\xef\xbb\xbf# User reference\r\nKeep my project notes." + if user_reference: + reference.parent.mkdir() + reference.write_bytes(original) + + args = ["preset", "remove", "my-preset"] if operation == "remove" else ["integration", "switch", "claude"] + result = runner.invoke(app, args, catch_exceptions=False) + + assert result.exit_code == 0, result.output + assert not (skill_dir / "SKILL.md").exists() + if user_reference: + assert reference.is_file() + assert reference.read_bytes() == original + else: + assert not skill_dir.exists() + + +@pytest.mark.parametrize("operation", ["remove", "switch"]) +@pytest.mark.parametrize("user_reference", [False, True]) +def test_preset_skill_cleanup_rollback_preserves_directory_mode( + tmp_path, monkeypatch, capsys, operation, user_reference, +): + from specify_cli.integrations._helpers import _read_integration_json + from specify_cli.integrations._lifecycle import _FileJournal, _transaction + from specify_cli.integrations.installer import read_records + + project = tmp_path / "project" + project.mkdir() + (tmp_path / "home").mkdir() + monkeypatch.chdir(project) + monkeypatch.setenv("HOME", str(tmp_path / "home")) + monkeypatch.setenv("USERPROFILE", str(tmp_path / "home")) + runner = CliRunner() + preset = Path(__file__).resolve().parents[3] / "presets" / "scaffold" + for args in ( + ["init", "--here", "--integration", "copilot", "--script", "sh", "--ignore-agent-tools"], + ["preset", "add", "--dev", str(preset)], + ): + result = runner.invoke(app, args, catch_exceptions=False) + assert result.exit_code == 0, result.output + skill_dir = project / ".github/skills/speckit-myext-myextcmd" + skill_file = skill_dir / "SKILL.md" + original = skill_file.read_bytes() + skill_dir.chmod(0o700) + reference = skill_dir / "references/user.md" + if user_reference: + reference.parent.mkdir() + reference.write_bytes(b"User reference before cleanup") + + snapshot = _FileJournal._snapshot + snapshotted = [] + + def snapshot_without_user_files(self, source, destination): + snapshotted.append(source) + assert source != reference and source != reference.parent + if source == skill_dir and user_reference: + assert not any(source.iterdir()) + return snapshot(self, source, destination) + + monkeypatch.setattr(_FileJournal, "_snapshot", snapshot_without_user_files) + args = ["preset", "remove", "my-preset"] if operation == "remove" else ["integration", "switch", "claude"] + previous_umask = os.umask(0o022) + try: + with pytest.raises(RuntimeError, match="failure after cleanup"): + with _transaction(project, "claude", _read_integration_json(project), read_records(project)): + result = runner.invoke(app, args, catch_exceptions=False) + assert result.exit_code == 0, result.output + assert not skill_file.exists() + assert skill_dir.exists() == user_reference + if user_reference: + reference.write_bytes(b"User reference edited during transaction") + raise RuntimeError("failure after cleanup") + finally: + os.umask(previous_umask) + + assert skill_file.read_bytes() == original + if os.name != "nt": + assert stat.S_IMODE(skill_dir.stat().st_mode) == 0o700 + if user_reference: + assert skill_dir not in snapshotted + assert reference.read_bytes() == b"User reference edited during transaction" + assert "Recovery snapshots retained" not in capsys.readouterr().out