diff --git a/CHANGELOG.md b/CHANGELOG.md index 52cf18db1..b2b4b17ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- `apm install` now removes orphaned skill directories when a package is uninstalled or its skills are renamed. Previously, individual files were deleted but the skill folder remained with a "Refused to remove directory entry" warning. (closes #1483) (#1767) + ## [0.20.0] - 2026-06-11 ### Added diff --git a/src/apm_cli/integration/cleanup.py b/src/apm_cli/integration/cleanup.py index 36e162de2..8e6fa6d92 100644 --- a/src/apm_cli/integration/cleanup.py +++ b/src/apm_cli/integration/cleanup.py @@ -10,11 +10,13 @@ 1. **Path validation** -- :meth:`BaseIntegrator.validate_deploy_path` rejects path traversal and any path not under a known integration prefix. -2. **Directory rejection** -- APM-managed primitives are file-keyed +2. **Directory handling** -- APM-managed primitives are file-keyed (``SKILL.md`` for skills, individual ``.prompt.md`` / ``.instructions.md`` - files elsewhere). A ``deployed_files`` entry that resolves to a directory - on disk is treated as untrusted (likely a poisoned lockfile entry under a - broad prefix like ``.github/instructions/``) and refused. + files elsewhere). Directory entries that match a known skill directory + pattern (``/skills/``) are deferred to a second pass so + individual files are removed first; if the directory is then empty (or + only contains APM-tracked files with matching hashes) it is safely + removed. Non-skill directory entries are still rejected as untrusted. 3. **Provenance check** -- when the previous lockfile recorded a content hash for the file, the on-disk content must still match. If the user edited the file after APM deployed it the hash will differ and the @@ -31,6 +33,7 @@ from __future__ import annotations +import shutil from collections.abc import Iterable from dataclasses import dataclass, field from pathlib import Path @@ -62,6 +65,186 @@ class CleanupResult: :meth:`BaseIntegrator.cleanup_empty_parents`.""" +def _is_skill_directory_entry(rel_path: str) -> bool: + """Return True when *rel_path* matches a skill directory pattern. + + Skill directories are deployed under ``/skills/`` where + ```` is a target root or deploy_root (e.g. ``.agents``, + ``.github``, ``.claude``, ``.cursor``). The path must have exactly + one component after ``skills/`` (the skill name) to qualify -- deeper + entries or ``skills/`` itself do not match. + """ + parts = Path(rel_path).parts + # Minimum: prefix, "skills", name -> 3 parts (e.g. ".agents/skills/my-skill") + if len(parts) < 3: + return False + # The second-to-last component must be "skills" and the last is the + # skill name. We require exactly one component after skills/ so that + # we only match the top-level skill directory, not subdirectories + # within a skill bundle. + try: + skills_idx = parts.index("skills") + except ValueError: + return False + # Exactly one component after "skills" (the skill name) + return skills_idx == len(parts) - 2 and skills_idx >= 1 + + +def _safe_remove_skill_directory( + skill_dir: Path, + rel_path: str, + recorded_hashes: dict[str, str], + all_stale: set[str], + result: CleanupResult, + diagnostics, + dep_key: str, + failed_path_retained: bool, +) -> None: + """Attempt safe removal of an APM-deployed skill directory. + + Called in the deferred second pass after individual files have been + deleted. The directory is removed when: + + - It no longer exists (already cleaned or never created). + - It is empty (all files already deleted in the first pass). + - All remaining files are tracked in *recorded_hashes* with matching + content hashes (APM-owned, unmodified). + + When the directory contains user-created or user-edited files, removal + is skipped with a diagnostic listing the blocking entries. + """ + if not skill_dir.exists(): + return + + if not skill_dir.is_dir() or skill_dir.is_symlink(): + return + + # Check if directory is empty -- safe to rmdir. + try: + remaining = list(skill_dir.iterdir()) + except OSError: + result.failed.append(rel_path) + return + + if not remaining: + try: + skill_dir.rmdir() + result.deleted.append(rel_path) + result.deleted_targets.append(skill_dir) + except OSError as exc: + result.failed.append(rel_path) + _emit_dir_failure(diagnostics, rel_path, exc, dep_key, failed_path_retained) + return + + # Non-empty: verify every remaining file is APM-tracked with + # matching hashes. Recurse into subdirectories (skills can + # contain scripts/, assets/, references/ subdirectories). + blocking: list[str] = [] + project_root = skill_dir + # Walk up to reconstruct project_root from the rel_path + rel_parts = Path(rel_path).parts + project_root = skill_dir + for _ in rel_parts: + project_root = project_root.parent + + for child in skill_dir.rglob("*"): + # Symlinks are treated as unmanaged -- APM does not deploy + # symlinks, so any symlink is user-created content. + if child.is_symlink(): + try: + child_rel = child.relative_to(project_root).as_posix() + except ValueError: + child_rel = str(child.name) + blocking.append(child_rel) + continue + if child.is_dir(): + continue + try: + child_rel = child.relative_to(project_root).as_posix() + except ValueError: + blocking.append(str(child.name)) + continue + expected_hash = recorded_hashes.get(child_rel) + if expected_hash is None: + # File not tracked by APM -- could be user-created. + # Check if it was in the stale set (already processed + # and potentially deleted). If not in stale set at all, + # it is definitely user content. + if child_rel not in all_stale: + blocking.append(child_rel) + continue + # Was in stale set but file still exists -- might have + # been skipped (user-edit, failed unlink). Block removal. + blocking.append(child_rel) + continue + # Verify hash matches. + try: + from ..utils.content_hash import compute_file_hash + + actual_hash = compute_file_hash(child) + except Exception: + blocking.append(child_rel) + continue + _strip = _strip_sha256_prefix + if _strip(actual_hash) != _strip(expected_hash): + blocking.append(child_rel) + + if blocking: + # Show up to 5 blocking paths so the user knows which files + # prevented cleanup (reviewer feedback on PR #1767). + preview = blocking[:5] + suffix = f" (and {len(blocking) - 5} more)" if len(blocking) > 5 else "" + file_list = ", ".join(preview) + suffix + diagnostics.warn( + ( + f"Skipped removing skill directory {rel_path}: " + f"{len(blocking)} file(s) not owned by APM or " + f"modified since deployment: {file_list}. " + "Remove manually if no longer needed." + ), + package=dep_key, + ) + result.skipped_unmanaged.append(rel_path) + return + + # All remaining files are APM-tracked with matching hashes -- safe + # to remove the entire directory tree. + try: + shutil.rmtree(skill_dir) + result.deleted.append(rel_path) + result.deleted_targets.append(skill_dir) + except OSError as exc: + result.failed.append(rel_path) + _emit_dir_failure(diagnostics, rel_path, exc, dep_key, failed_path_retained) + + +def _emit_dir_failure(diagnostics, rel_path, exc, dep_key, failed_path_retained): + """Emit a diagnostic for a failed directory removal.""" + if failed_path_retained: + diagnostics.warn( + ( + f"Could not remove skill directory {rel_path}: {exc}. " + "Path retained in lockfile; will retry on next " + "'apm install'." + ), + package=dep_key, + ) + else: + diagnostics.warn( + ( + f"Could not remove skill directory {rel_path}: {exc}. " + "The owning package is no longer in apm.yml -- " + "remove the directory manually." + ), + package=dep_key, + ) + + +def _strip_sha256_prefix(h: str) -> str: + """Strip the ``sha256:`` prefix for hash comparison.""" + return h[len("sha256:") :] if h.startswith("sha256:") else h + + def remove_stale_deployed_files( stale_paths: Iterable[str], project_root: Path, @@ -115,6 +298,18 @@ def remove_stale_deployed_files( result = CleanupResult() recorded_hashes = recorded_hashes or {} + # Materialise stale_paths so we can iterate twice: once for the main + # file-deletion loop and once as a lookup set for the deferred + # directory pass. + _stale_list = sorted(stale_paths) + _stale_set: set[str] = set(_stale_list) + + # Skill directory entries are deferred to a second pass so individual + # files are removed first. After files are deleted the directory is + # either empty (safe rmdir) or contains only verified APM-tracked + # files (safe rmtree). + _deferred_dirs: list[tuple[str, Path]] = [] + # Lazy-resolve cowork root at most once per invocation (same # pattern as sync_remove_files in base_integrator.py -- PR #926 P4). _cowork_root_resolved: bool = False @@ -122,8 +317,8 @@ def remove_stale_deployed_files( _cowork_orphans_skipped: int = 0 _cowork_resolve_errors: int = 0 - for stale_path in sorted(stale_paths): - # ── Cowork:// paths ────────────────────────────────────────── + for stale_path in _stale_list: + # -- Cowork:// paths --------------------------------------- # Handled BEFORE validate_deploy_path because that method # hard-rejects cowork:// when the OneDrive root is unavailable # (returning False ⇒ skipped_unmanaged). For cleanup we want @@ -179,21 +374,27 @@ def remove_stale_deployed_files( # File already gone -- treat as cleaned (no-op success). continue - # Gate 2: directory rejection. APM-managed primitives are + # Gate 2: directory handling. APM-managed primitives are # file-keyed; a directory entry under an integration prefix is - # almost certainly a poisoned lockfile entry that would rmtree - # an entire user-managed subtree. + # normally treated as untrusted. However, skill directories are + # legitimately tracked by APM (services.py records the skill dir + # entry alongside its contained files). Skill directories are + # deferred to a second pass so individual files are deleted first; + # non-skill directory entries are still rejected immediately. if stale_target.is_dir() and not stale_target.is_symlink(): - result.skipped_unmanaged.append(stale_path) - diagnostics.warn( - ( - f"Refused to remove directory entry {stale_path}: APM " - "only deletes individual files. If this entry was added " - "by a malicious or corrupt lockfile, remove it manually " - "from apm.lock.yaml." - ), - package=dep_key, - ) + if _is_skill_directory_entry(stale_path): + _deferred_dirs.append((stale_path, stale_target)) + else: + result.skipped_unmanaged.append(stale_path) + diagnostics.warn( + ( + f"Refused to remove directory entry {stale_path}: APM " + "only deletes individual files. If this entry was added " + "by a malicious or corrupt lockfile, remove it manually " + "from apm.lock.yaml." + ), + package=dep_key, + ) continue # Gate 3: provenance check. If APM recorded a content hash for @@ -227,10 +428,7 @@ def remove_stale_deployed_files( # 0.12.0 fix). ``compute_file_hash`` always returns the # prefixed form, so strip the prefix from both sides before # comparing to avoid false "user-edited" classifications. - def _strip_sha256(h: str) -> str: - return h[len("sha256:") :] if h.startswith("sha256:") else h - - if _strip_sha256(actual_hash) != _strip_sha256(expected_hash): + if _strip_sha256_prefix(actual_hash) != _strip_sha256_prefix(expected_hash): result.skipped_user_edit.append(stale_path) diagnostics.warn( ( @@ -269,6 +467,21 @@ def _strip_sha256(h: str) -> str: package=dep_key, ) + # -- Second pass: deferred skill directories ------------------- + # Individual files have been deleted above. Now attempt safe removal + # of skill directories that APM itself created. + for _dir_path, _dir_target in _deferred_dirs: + _safe_remove_skill_directory( + _dir_target, + _dir_path, + recorded_hashes, + _stale_set, + result, + diagnostics, + dep_key, + failed_path_retained, + ) + # One-time warnings for cowork edge cases (mirrors sync_remove_files). if _cowork_orphans_skipped > 0: diagnostics.warn( diff --git a/tests/unit/integration/test_cleanup_skill_dirs.py b/tests/unit/integration/test_cleanup_skill_dirs.py new file mode 100644 index 000000000..6a2a5f8ec --- /dev/null +++ b/tests/unit/integration/test_cleanup_skill_dirs.py @@ -0,0 +1,307 @@ +"""Unit tests for skill directory cleanup in ``remove_stale_deployed_files``. + +Tests the deferred skill directory removal feature (issue #1483): when a +package is removed and its deployed files include a skill directory entry, +APM should safely remove that directory after deleting individual files +instead of emitting a "Refused to remove directory entry" warning. +""" + +from pathlib import Path + +import pytest + +from apm_cli.integration.cleanup import ( + _is_skill_directory_entry, + remove_stale_deployed_files, +) +from apm_cli.utils.content_hash import compute_file_hash +from apm_cli.utils.diagnostics import DiagnosticCollector + + +@pytest.fixture +def project_root(tmp_path): + return tmp_path + + +@pytest.fixture +def diagnostics(): + return DiagnosticCollector(verbose=False) + + +def _make_file(root: Path, rel: str, content: str = "hello\n") -> Path: + p = root / rel + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(content, encoding="utf-8") + return p + + +# ------------------------------------------------------------------ +# _is_skill_directory_entry +# ------------------------------------------------------------------ + + +class TestIsSkillDirectoryEntry: + def test_standard_agents_skill(self): + assert _is_skill_directory_entry(".agents/skills/my-skill") + + def test_github_skills(self): + assert _is_skill_directory_entry(".github/skills/coding-lint") + + def test_claude_skills(self): + assert _is_skill_directory_entry(".claude/skills/my-tool") + + def test_cursor_skills(self): + assert _is_skill_directory_entry(".cursor/skills/helper") + + def test_too_short_rejected(self): + assert not _is_skill_directory_entry("skills/name") + + def test_skills_root_rejected(self): + assert not _is_skill_directory_entry(".agents/skills") + + def test_subdir_within_skill_rejected(self): + assert not _is_skill_directory_entry(".agents/skills/my-skill/scripts") + + def test_non_skill_directory(self): + assert not _is_skill_directory_entry(".github/instructions") + + def test_prompts_dir_rejected(self): + assert not _is_skill_directory_entry(".github/prompts") + + def test_no_skills_component(self): + assert not _is_skill_directory_entry(".github/agents/my-agent") + + +# ------------------------------------------------------------------ +# Skill directory removal in remove_stale_deployed_files +# ------------------------------------------------------------------ + + +class TestSkillDirectoryCleanup: + def test_empty_skill_dir_removed_after_files_deleted(self, project_root, diagnostics): + """Skill dir is empty after individual files are deleted -> rmdir.""" + skill_md = _make_file(project_root, ".agents/skills/my-skill/SKILL.md", "# My Skill\n") + # Skill dir entry + file entry, both stale + stale = [ + ".agents/skills/my-skill", + ".agents/skills/my-skill/SKILL.md", + ] + result = remove_stale_deployed_files( + stale, + project_root, + dep_key="pkg", + targets=None, + diagnostics=diagnostics, + ) + assert ".agents/skills/my-skill/SKILL.md" in result.deleted + assert ".agents/skills/my-skill" in result.deleted + assert not skill_md.exists() + assert not (project_root / ".agents/skills/my-skill").exists() + # No "Refused" warnings + msgs = [d.message for d in diagnostics._diagnostics] + assert not any("Refused to remove directory entry" in m for m in msgs) + + def test_skill_dir_with_assets_removed_when_hashes_match(self, project_root, diagnostics): + """Skill dir with remaining APM-tracked files is rmtree'd. + + The asset file is NOT in the stale list, so it remains on disk + after the first pass. The second pass checks its hash and removes + the whole directory tree via rmtree. + """ + skill_md = _make_file(project_root, ".agents/skills/my-skill/SKILL.md", "# Skill\n") + asset = _make_file(project_root, ".agents/skills/my-skill/assets/data.json", '{"a":1}\n') + # Record hashes for both files + recorded_hashes = { + ".agents/skills/my-skill/SKILL.md": compute_file_hash(skill_md), + ".agents/skills/my-skill/assets/data.json": compute_file_hash(asset), + } + # Only the dir entry and SKILL.md are stale -- asset is NOT in the + # stale list, so it remains after the first pass and the second pass + # must exercise the rmtree + hash verification path. + stale = [ + ".agents/skills/my-skill", + ".agents/skills/my-skill/SKILL.md", + ] + result = remove_stale_deployed_files( + stale, + project_root, + dep_key="pkg", + targets=None, + diagnostics=diagnostics, + recorded_hashes=recorded_hashes, + ) + assert ".agents/skills/my-skill" in result.deleted + assert not (project_root / ".agents/skills/my-skill").exists() + + def test_skill_dir_not_removed_when_user_file_present(self, project_root, diagnostics): + """Skill dir is NOT removed when it contains user-created files.""" + _make_file(project_root, ".agents/skills/my-skill/SKILL.md", "# Skill\n") + # User adds their own file in the skill directory + _make_file(project_root, ".agents/skills/my-skill/my-notes.txt", "user notes\n") + stale = [ + ".agents/skills/my-skill", + ".agents/skills/my-skill/SKILL.md", + ] + result = remove_stale_deployed_files( + stale, + project_root, + dep_key="pkg", + targets=None, + diagnostics=diagnostics, + ) + # SKILL.md is deleted (it is a file entry) + assert ".agents/skills/my-skill/SKILL.md" in result.deleted + # But directory is skipped because of user file + assert ".agents/skills/my-skill" in result.skipped_unmanaged + # User file intact + assert (project_root / ".agents/skills/my-skill/my-notes.txt").exists() + msgs = [d.message for d in diagnostics._diagnostics] + assert any("not owned by APM" in m for m in msgs) + # Diagnostic should list the blocking file path + assert any("my-notes.txt" in m for m in msgs) + + def test_skill_dir_not_removed_when_hash_mismatch(self, project_root, diagnostics): + """Skill dir is NOT removed when a tracked file was user-edited.""" + _make_file(project_root, ".agents/skills/my-skill/SKILL.md", "# Skill\n") + asset = _make_file( + project_root, + ".agents/skills/my-skill/references/guide.md", + "edited content\n", + ) + # Recorded hash does NOT match current content + recorded_hashes = { + ".agents/skills/my-skill/references/guide.md": "sha256:" + "0" * 64, + } + stale = [ + ".agents/skills/my-skill", + ".agents/skills/my-skill/SKILL.md", + ".agents/skills/my-skill/references/guide.md", + ] + result = remove_stale_deployed_files( + stale, + project_root, + dep_key="pkg", + targets=None, + diagnostics=diagnostics, + recorded_hashes=recorded_hashes, + ) + # SKILL.md deleted (no hash recorded -> falls through) + assert ".agents/skills/my-skill/SKILL.md" in result.deleted + # guide.md skipped (hash mismatch -> user edit) + assert ".agents/skills/my-skill/references/guide.md" in result.skipped_user_edit + # Directory skipped because guide.md is still there with mismatch + assert ".agents/skills/my-skill" in result.skipped_unmanaged + assert asset.exists() + + def test_non_skill_dir_still_rejected(self, project_root, diagnostics): + """Non-skill directory entries still get the old rejection.""" + (project_root / ".github" / "prompts").mkdir(parents=True) + _make_file(project_root, ".github/prompts/user.prompt.md", "user content\n") + result = remove_stale_deployed_files( + [".github/prompts"], + project_root, + dep_key="pkg", + targets=None, + diagnostics=diagnostics, + ) + assert result.deleted == [] + assert ".github/prompts" in result.skipped_unmanaged + msgs = [d.message for d in diagnostics._diagnostics] + assert any("Refused to remove directory entry" in m for m in msgs) + + def test_skill_dir_already_gone(self, project_root, diagnostics): + """Skill dir entry for a non-existent directory is a no-op.""" + result = remove_stale_deployed_files( + [".agents/skills/gone-skill"], + project_root, + dep_key="pkg", + targets=None, + diagnostics=diagnostics, + ) + # Not an error, not a warning -- just silently clean + assert result.deleted == [] + assert result.failed == [] + assert result.skipped_unmanaged == [] + + def test_backward_compat_no_hashes_empty_dir_removed(self, project_root, diagnostics): + """Legacy lockfile without hashes: skill dir removed when empty.""" + _make_file(project_root, ".agents/skills/old-skill/SKILL.md", "# Old\n") + stale = [ + ".agents/skills/old-skill", + ".agents/skills/old-skill/SKILL.md", + ] + result = remove_stale_deployed_files( + stale, + project_root, + dep_key="pkg", + targets=None, + diagnostics=diagnostics, + recorded_hashes=None, + ) + assert ".agents/skills/old-skill/SKILL.md" in result.deleted + assert ".agents/skills/old-skill" in result.deleted + assert not (project_root / ".agents/skills/old-skill").exists() + + def test_skill_dir_with_subdirs_removed_when_all_tracked(self, project_root, diagnostics): + """Skill bundle with scripts/ and assets/ subdirs, all tracked.""" + skill_md = _make_file(project_root, ".agents/skills/full-skill/SKILL.md", "# Full\n") + script = _make_file(project_root, ".agents/skills/full-skill/scripts/run.sh", "#!/bin/sh\n") + asset = _make_file(project_root, ".agents/skills/full-skill/assets/img.png", "PNG\n") + recorded_hashes = { + ".agents/skills/full-skill/SKILL.md": compute_file_hash(skill_md), + ".agents/skills/full-skill/scripts/run.sh": compute_file_hash(script), + ".agents/skills/full-skill/assets/img.png": compute_file_hash(asset), + } + stale = [ + ".agents/skills/full-skill", + ".agents/skills/full-skill/SKILL.md", + ".agents/skills/full-skill/scripts/run.sh", + ".agents/skills/full-skill/assets/img.png", + ] + result = remove_stale_deployed_files( + stale, + project_root, + dep_key="pkg", + targets=None, + diagnostics=diagnostics, + recorded_hashes=recorded_hashes, + ) + assert ".agents/skills/full-skill" in result.deleted + assert not (project_root / ".agents/skills/full-skill").exists() + + def test_path_traversal_in_skill_dir_still_rejected(self, project_root, diagnostics): + """Path traversal in a skill directory name is caught by Gate 1.""" + result = remove_stale_deployed_files( + [".agents/skills/../../../etc"], + project_root, + dep_key="pkg", + targets=None, + diagnostics=diagnostics, + ) + assert result.deleted == [] + assert result.skipped_unmanaged == [".agents/skills/../../../etc"] + + def test_symlink_inside_skill_dir_blocks_removal(self, project_root, diagnostics): + """Symlinks inside a skill directory are treated as user content.""" + _make_file(project_root, ".agents/skills/my-skill/SKILL.md", "# Skill\n") + # Create a symlink inside the skill directory + link = project_root / ".agents/skills/my-skill/link.txt" + target = project_root / "some-other-file.txt" + target.write_text("target content\n", encoding="utf-8") + link.symlink_to(target) + stale = [ + ".agents/skills/my-skill", + ".agents/skills/my-skill/SKILL.md", + ] + result = remove_stale_deployed_files( + stale, + project_root, + dep_key="pkg", + targets=None, + diagnostics=diagnostics, + ) + assert ".agents/skills/my-skill/SKILL.md" in result.deleted + assert ".agents/skills/my-skill" in result.skipped_unmanaged + # Symlink and its target remain + assert link.is_symlink() + assert target.exists()