fix(skills): recognize fully deleted public skill packages in review CI (#4169)

select_skill_packages() resolved every changed non-SKILL.md path to its
owning package via an unconditional depth-3 fallback, then queued that
path for review. When a PR deletes an entire public skill package (not
just SKILL.md, but its scripts/assets/etc. too), the fallback still
returned the package directory even though it no longer exists on disk
post-deletion. The review CLI then reported a false
structure.missing-skill-md blocker for a path that isn't there,
failing CI on a routine, correct package removal.

Skip a resolved package only when every changed file under it was a
deletion and the package directory itself is gone from disk - i.e. the
whole package was intentionally removed. A package left in a
broken/partial state (e.g. SKILL.md deleted while sibling files
remain) still resolves to an existing directory, so it is unaffected
and continues to be queued and flagged.

backend/tests/test_review_changed_public_skills.py

@@ -1,7 +1,7 @@
 from __future__ import annotations
 
 import subprocess
-from pathlib import Path
+from pathlib import Path, PurePosixPath
 
 import review_changed_public_skills as runner
 
@@ -111,6 +111,98 @@ def test_main_reviews_changed_public_skill_and_skips_deleted_skill_md(
     assert "All changed public skill packages passed review." in output
 
 
+def test_main_skips_fully_deleted_skill_package(tmp_path: Path, monkeypatch, capsys) -> None:
+    # Nothing is written to tmp_path for "removed": the whole package (SKILL.md and its
+    # other files) was deleted, so the package directory does not exist on disk anymore.
+    diff_output = b"\0".join(
+        [
+            b"D",
+            b"skills/public/removed/SKILL.md",
+            b"D",
+            b"skills/public/removed/scripts/helper.py",
+            b"D",
+            b"skills/public/removed/assets/logo.png",
+            b"",
+        ]
+    )
+
+    def fake_git_diff(command, **kwargs):
+        return _completed(command, stdout=diff_output)
+
+    def fail_review(*args, **kwargs):
+        raise AssertionError("review should not run for a fully deleted skill package")
+
+    monkeypatch.setattr(runner.subprocess, "run", fake_git_diff)
+    monkeypatch.setattr(runner, "run_review", fail_review)
+
+    exit_code = runner.main(
+        [
+            "--before",
+            "before",
+            "--after",
+            "after",
+            "--repo-root",
+            str(tmp_path),
+        ]
+    )
+
+    output = capsys.readouterr().out
+    assert exit_code == 0
+    assert "Skipping deleted SKILL.md: skills/public/removed/SKILL.md" in output
+    assert "Skipping fully removed package: skills/public/removed" in output
+    assert "No changed public skill package files; skipping review." in output
+
+
+def test_main_reviews_package_when_skill_md_deleted_but_sibling_file_remains(
+    tmp_path: Path,
+    monkeypatch,
+    capsys,
+) -> None:
+    # SKILL.md was deleted but a sibling package file still exists on disk: this is a
+    # broken/partial package, not a full removal, and must still be queued for review.
+    skill_dir = tmp_path / "skills" / "public" / "broken"
+    (skill_dir / "scripts").mkdir(parents=True, exist_ok=True)
+    (skill_dir / "scripts" / "helper.py").write_text("def helper():\n    return 1\n", encoding="utf-8")
+
+    diff_output = b"\0".join(
+        [
+            b"D",
+            b"skills/public/broken/SKILL.md",
+            b"M",
+            b"skills/public/broken/scripts/helper.py",
+            b"",
+        ]
+    )
+    reviewed: list[str] = []
+
+    def fake_git_diff(command, **kwargs):
+        return _completed(command, stdout=diff_output)
+
+    def fake_review(package: Path, repo_root: Path, python_executable: str) -> int:
+        reviewed.append(package.relative_to(repo_root).as_posix())
+        return 1
+
+    monkeypatch.setattr(runner.subprocess, "run", fake_git_diff)
+    monkeypatch.setattr(runner, "run_review", fake_review)
+
+    exit_code = runner.main(
+        [
+            "--before",
+            "before",
+            "--after",
+            "after",
+            "--repo-root",
+            str(tmp_path),
+        ]
+    )
+
+    output = capsys.readouterr().out
+    assert exit_code == 1
+    assert reviewed == ["skills/public/broken"]
+    assert "Queued package: skills/public/broken" in output
+    assert "One or more skill reviews failed." in output
+
+
 def test_main_reviews_package_when_only_support_file_changed(
     tmp_path: Path,
     monkeypatch,
@@ -267,6 +359,22 @@ def test_main_falls_back_to_empty_tree_when_push_before_is_missing(tmp_path: Pat
     assert "Fallback diff:" in output
 
 
+def test_is_fully_removed_package_true_when_all_deletions_and_directory_missing(tmp_path: Path) -> None:
+    package_rel = PurePosixPath("skills/public/removed")
+    assert runner.is_fully_removed_package(package_rel, ["D", "D"], tmp_path) is True
+
+
+def test_is_fully_removed_package_false_when_directory_still_exists(tmp_path: Path) -> None:
+    package_rel = PurePosixPath("skills/public/broken")
+    (tmp_path / package_rel).mkdir(parents=True)
+    assert runner.is_fully_removed_package(package_rel, ["D", "D"], tmp_path) is False
+
+
+def test_is_fully_removed_package_false_when_any_status_is_not_a_deletion(tmp_path: Path) -> None:
+    package_rel = PurePosixPath("skills/public/partial")
+    assert runner.is_fully_removed_package(package_rel, ["D", "M"], tmp_path) is False
+
+
 def test_is_zero_sha_requires_full_sha_length() -> None:
     assert runner.is_zero_sha("0" * 40) is True
     assert runner.is_zero_sha("0" * 64) is True

scripts/review_changed_public_skills.py

@@ -161,8 +161,8 @@ def parse_name_status(output: bytes) -> list[ChangedPath]:
 
 
 def select_skill_packages(changes: Sequence[ChangedPath], repo_root: Path) -> list[Path]:
-    packages: list[Path] = []
-    seen: set[PurePosixPath] = set()
+    package_statuses: dict[PurePosixPath, list[str]] = {}
+    resolutions: list[tuple[ChangedPath, PurePosixPath]] = []
 
     for change in changes:
         if not is_public_skill_package_path(change.path):
@@ -177,17 +177,42 @@ def select_skill_packages(changes: Sequence[ChangedPath], repo_root: Path) -> li
             print(f"[skill-review] Skipping path outside public skill package: {change.path}")
             continue
 
+        package_statuses.setdefault(package_rel, []).append(change.status)
+        resolutions.append((change, package_rel))
+
+    packages: list[Path] = []
+    seen: set[PurePosixPath] = set()
+
+    for _, package_rel in resolutions:
         if package_rel in seen:
             print(f"[skill-review] Already queued package: {package_rel}")
             continue
-
         seen.add(package_rel)
+
+        if is_fully_removed_package(package_rel, package_statuses[package_rel], repo_root):
+            print(f"[skill-review] Skipping fully removed package: {package_rel}")
+            continue
+
         packages.append(repo_root / package_rel)
         print(f"[skill-review] Queued package: {package_rel}")
 
     return packages
 
 
+def is_fully_removed_package(package_rel: PurePosixPath, statuses: Sequence[str], repo_root: Path) -> bool:
+    """Whether every changed file that resolved to ``package_rel`` was a deletion and the
+    package directory itself no longer exists on disk.
+
+    This identifies a whole public skill package being intentionally deleted (all of its
+    files removed, not just SKILL.md), as distinct from a package left in a broken/partial
+    state (e.g. SKILL.md deleted while other package files remain on disk) — the latter
+    must still be reviewed and flagged.
+    """
+    if not all(status.startswith("D") for status in statuses):
+        return False
+    return not (repo_root / package_rel).is_dir()
+
+
 def is_public_skill_md(path: PurePosixPath) -> bool:
     parts = path.parts
     return len(parts) >= 4 and parts[0] == "skills" and parts[1] == "public" and parts[-1] == "SKILL.md" and not _is_eval_fixture_skill_md(path)