From 656f6b364c30b1137b84bf0d8f01002bab5eaefe Mon Sep 17 00:00:00 2001 From: Daoyuan Li <94409450+DaoyuanLi2816@users.noreply.github.com> Date: Tue, 14 Jul 2026 08:08:33 -0700 Subject: [PATCH] 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. --- .../test_review_changed_public_skills.py | 110 +++++++++++++++++- scripts/review_changed_public_skills.py | 31 ++++- 2 files changed, 137 insertions(+), 4 deletions(-) diff --git a/backend/tests/test_review_changed_public_skills.py b/backend/tests/test_review_changed_public_skills.py index ad159b527..5b2ef16f4 100644 --- a/backend/tests/test_review_changed_public_skills.py +++ b/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 diff --git a/scripts/review_changed_public_skills.py b/scripts/review_changed_public_skills.py index 89813f87e..f944c864d 100644 --- a/scripts/review_changed_public_skills.py +++ b/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)