From d62d35ae6e02c64106ac58c06dc88eb0a1390ce5 Mon Sep 17 00:00:00 2001 From: shaurya2k06 Date: Mon, 27 Jul 2026 18:34:39 +0530 Subject: [PATCH] fix(skill-and-tool-validator): harden three non-blocking review leftovers Correct the license-size docstring to bytes, skip the live header integration out-of-tree instead of aborting, and catch OSError from validate_eval_coverage directory listing the same way collect_tool_python_files does. Also re-lock root uv.lock mypy requires-dev specifiers to >=2.3.0 so prek stops rewriting the lock when this tool is touched (same correction as apache/magpie#965). Signed-off-by: shaurya2k06 --- .../src/skill_and_tool_validator/__init__.py | 20 ++++-- .../tests/test_validator.py | 66 ++++++++++++++++++- uv.lock | 16 ++--- 3 files changed, 88 insertions(+), 14 deletions(-) diff --git a/tools/skill-and-tool-validator/src/skill_and_tool_validator/__init__.py b/tools/skill-and-tool-validator/src/skill_and_tool_validator/__init__.py index 4376989c..be92f262 100644 --- a/tools/skill-and-tool-validator/src/skill_and_tool_validator/__init__.py +++ b/tools/skill-and-tool-validator/src/skill_and_tool_validator/__init__.py @@ -2469,8 +2469,8 @@ def validate_lowercase_f_field(path: Path, text: str) -> Iterable[Violation]: "apache.org/licenses/LICENSE-2.0", ) -# Files smaller than this threshold (bytes / characters) are treated as -# empty placeholder stubs and exempted from the license-header check. +# Files smaller than this threshold (bytes) are treated as empty +# placeholder stubs and exempted from the license-header check. _MIN_LICENSE_FILE_SIZE = 50 # Path components that mark generated or vendored subtrees that must not @@ -2485,7 +2485,7 @@ def collect_tool_python_files(root: Path | None = None) -> list[Path]: Excludes generated / vendored subtrees (``.venv``, ``site-packages``, ``node_modules``, ``__pycache__``) and empty placeholder files whose - content is shorter than ``_MIN_LICENSE_FILE_SIZE`` characters. + content is shorter than ``_MIN_LICENSE_FILE_SIZE`` bytes. """ base = (root or find_repo_root()) / TOOLS_DIR if not base.exists(): @@ -3297,8 +3297,18 @@ def validate_eval_coverage(root: Path | None = None) -> Iterable[Violation]: return eval_slugs: set[str] = set() if evals_base.exists(): - eval_slugs = {p.name for p in evals_base.iterdir() if p.is_dir()} - for skill_dir in sorted(skills_base.iterdir()): + try: + eval_slugs = {p.name for p in evals_base.iterdir() if p.is_dir()} + except OSError: + # Restrictive runners can deny listing; skip rather than crash + # (same posture as collect_tool_python_files on OSError). + return + try: + skill_dirs = sorted(skills_base.iterdir()) + except OSError: + # Same posture as collect_tool_python_files: unreadable → skip. + return + for skill_dir in skill_dirs: if not skill_dir.is_dir(): continue # A trusted-external-skill-source pointer dir carries its eval suite diff --git a/tools/skill-and-tool-validator/tests/test_validator.py b/tools/skill-and-tool-validator/tests/test_validator.py index 5e33d4c4..90456d08 100644 --- a/tools/skill-and-tool-validator/tests/test_validator.py +++ b/tools/skill-and-tool-validator/tests/test_validator.py @@ -20,6 +20,7 @@ from __future__ import annotations import subprocess +from collections.abc import Iterator from pathlib import Path import pytest @@ -1595,6 +1596,21 @@ def test_lowercase_f_field_in_soft_categories(self) -> None: _SPDX_PY_HEADER = "# SPDX-License-Identifier: Apache-2.0\n" +def _live_repo_root_for_header_check() -> Path | None: + """Return the magpie repo root when the live license-header check can run. + + ``find_repo_root`` never raises — out-of-tree it returns CWD — so the + live integration must additionally confirm this checkout actually + contains the validator package under ``tools/``. Returns ``None`` when + that marker is absent so callers can ``skipif`` instead of aborting. + """ + root = find_repo_root() + marker = root / "tools" / "skill-and-tool-validator" / "src" / "skill_and_tool_validator" + if marker.is_dir(): + return root + return None + + class TestValidateLicenseHeader: # ------------------------------------------------------------------ # # Python (.py) checks # @@ -1683,11 +1699,23 @@ def test_collect_tool_python_files_returns_empty_when_no_tools_dir(self, tmp_pat # Integration: real repo passes # # ------------------------------------------------------------------ # + def test_live_repo_header_check_skips_out_of_tree( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Out-of-tree cwd must make the live header-check skipif fire (not error).""" + monkeypatch.chdir(tmp_path) + assert _live_repo_root_for_header_check() is None + + @pytest.mark.skipif( + _live_repo_root_for_header_check() is None, + reason="not running inside a magpie repo checkout with tools/", + ) def test_real_repo_tool_python_files_all_have_headers(self) -> None: """Every non-trivial tool Python file in the real repo carries a license header.""" from skill_and_tool_validator import _LICENSE_PY_MARKERS - repo_root = find_repo_root() + repo_root = _live_repo_root_for_header_check() + assert repo_root is not None # skipif above guarantees this missing = [ p for p in collect_tool_python_files(repo_root) @@ -3635,6 +3663,42 @@ def test_non_directory_entries_in_skills_are_skipped(self, tmp_path: Path) -> No violations = list(validate_eval_coverage(tmp_path)) assert violations == [] + def test_unreadable_skills_dir_returns_no_violations( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """PermissionError on skills/.iterdir() must not abort the run.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir(parents=True) + self._make_skill(tmp_path, "alpha") + + real_iterdir = Path.iterdir + + def _boom(self: Path) -> Iterator[Path]: + if self.resolve() == skills_dir.resolve(): + raise PermissionError("denied") + return real_iterdir(self) + + monkeypatch.setattr(Path, "iterdir", _boom) + assert list(validate_eval_coverage(tmp_path)) == [] + + def test_unreadable_evals_dir_returns_no_violations( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """PermissionError on evals/.iterdir() must not abort or false-flag skills.""" + self._make_skill(tmp_path, "alpha") + evals_dir = tmp_path / "tools" / "skill-evals" / "evals" + evals_dir.mkdir(parents=True) + + real_iterdir = Path.iterdir + + def _boom(self: Path) -> Iterator[Path]: + if self.resolve() == evals_dir.resolve(): + raise PermissionError("denied") + return real_iterdir(self) + + monkeypatch.setattr(Path, "iterdir", _boom) + assert list(validate_eval_coverage(tmp_path)) == [] + # --------------------------------------------------------------------------- # docs/modes.md consistency check (check #11 — SOFT) diff --git a/uv.lock b/uv.lock index b70568a4..f0fe7246 100644 --- a/uv.lock +++ b/uv.lock @@ -369,7 +369,7 @@ dev = [ [package.metadata.requires-dev] dev = [ - { name = "mypy", specifier = ">=2.1.0" }, + { name = "mypy", specifier = ">=2.3.0" }, { name = "pytest", specifier = ">=9.1.1" }, { name = "ruff", specifier = ">=0.15.21" }, ] @@ -495,7 +495,7 @@ dev = [ [package.metadata.requires-dev] dev = [ - { name = "mypy", specifier = ">=2.1.0" }, + { name = "mypy", specifier = ">=2.3.0" }, { name = "pytest", specifier = ">=9.1.1" }, { name = "ruff", specifier = ">=0.15.21" }, ] @@ -945,7 +945,7 @@ provides-extras = ["mcp"] [package.metadata.requires-dev] dev = [ { name = "mcp", specifier = ">=1.28.1" }, - { name = "mypy", specifier = ">=2.1.0" }, + { name = "mypy", specifier = ">=2.3.0" }, { name = "pytest", specifier = ">=9.1.1" }, { name = "ruff", specifier = ">=0.15.21" }, ] @@ -1359,7 +1359,7 @@ dev = [ [package.metadata.requires-dev] dev = [ - { name = "mypy", specifier = ">=2.1.0" }, + { name = "mypy", specifier = ">=2.3.0" }, { name = "pytest", specifier = ">=9.1.1" }, { name = "ruff", specifier = ">=0.15.21" }, ] @@ -1570,7 +1570,7 @@ dev = [ [package.metadata.requires-dev] dev = [ - { name = "mypy", specifier = ">=2.1.0" }, + { name = "mypy", specifier = ">=2.3.0" }, { name = "pytest", specifier = ">=9.1.1" }, { name = "ruff", specifier = ">=0.15.21" }, ] @@ -1612,7 +1612,7 @@ dev = [ [package.metadata.requires-dev] dev = [ - { name = "mypy", specifier = ">=2.1.0" }, + { name = "mypy", specifier = ">=2.3.0" }, { name = "pytest", specifier = ">=9.1.1" }, { name = "ruff", specifier = ">=0.15.21" }, ] @@ -1633,7 +1633,7 @@ dev = [ [package.metadata.requires-dev] dev = [ - { name = "mypy", specifier = ">=2.1.0" }, + { name = "mypy", specifier = ">=2.3.0" }, { name = "pytest", specifier = ">=9.1.1" }, { name = "ruff", specifier = ">=0.15.21" }, ] @@ -1843,7 +1843,7 @@ dev = [ [package.metadata.requires-dev] dev = [ - { name = "mypy", specifier = ">=2.1.0" }, + { name = "mypy", specifier = ">=2.3.0" }, { name = "pytest", specifier = ">=9.1.1" }, { name = "ruff", specifier = ">=0.15.21" }, ]