diff --git a/CHANGELOG.md b/CHANGELOG.md index 8673520..5f86489 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,9 @@ All notable changes to SkillEvaluator are documented in this file. ### Fixed +- Malformed, non-UTF-8, or unreadable bundled and custom policy files now + produce path-specific CLI errors instead of leaking raw parser or I/O errors + ([#128](https://github.com/NVIDIA/SkillEvaluator/issues/128)). - License detection no longer treats a frontmatter `license` identifier as authoritative when a LICENSE file declares a different license. Claiming MIT while shipping GPL-3.0 now fails closed. Every LICENSE/COPYING file is diff --git a/src/skillevaluator/validators/policy.py b/src/skillevaluator/validators/policy.py index e363435..89a09e9 100644 --- a/src/skillevaluator/validators/policy.py +++ b/src/skillevaluator/validators/policy.py @@ -161,6 +161,19 @@ def _coerce_email_regex(value: Any, source: str) -> re.Pattern[str] | None: _KNOWN_IDENTITY_KEYS = {"author_email_regex"} +def _load_policy_yaml(path: Path) -> Any: + """Load policy YAML and normalize parser failures to the public contract.""" + try: + with path.open(encoding="utf-8") as fh: + return yaml.safe_load(fh) or {} + except (yaml.YAMLError, UnicodeError, RecursionError) as exc: + raise ValueError(f"Invalid policy YAML in {path}: {exc}") from exc + except FileNotFoundError: + raise + except OSError as exc: + raise ValueError(f"Could not read policy file {path}: {exc}") from exc + + def _warn_unknown_keys(data: dict[str, Any], known: set[str], context: str, source: str) -> None: unknown = set(data) - known if unknown: @@ -227,8 +240,7 @@ def load_profile(name: str = DEFAULT_PROFILE_NAME) -> ValidationPolicy: if not path.exists(): available = sorted(p.stem for p in PROFILES_DIR.glob("*.yaml")) if PROFILES_DIR.exists() else [] raise FileNotFoundError(f"Unknown profile {name!r}. Available bundled profiles: {available or '(none)'}") - with path.open(encoding="utf-8") as fh: - data = yaml.safe_load(fh) or {} + data = _load_policy_yaml(path) return _policy_from_data(data, fallback_profile=name, source=path) @@ -248,8 +260,7 @@ def load_policy_file( raise FileNotFoundError(f"Custom policy file not found: {path}") base = load_profile(base_profile) - with path.open(encoding="utf-8") as fh: - custom_data = yaml.safe_load(fh) or {} + custom_data = _load_policy_yaml(path) custom = _policy_from_data(custom_data, fallback_profile=base.profile, source=path) merged_overrides = dict(base.severity_overrides) diff --git a/tests/test_commands.py b/tests/test_commands.py index b9e28b5..a91abe0 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -93,6 +93,29 @@ def test_validate_fixture_no_llm() -> None: assert "All validations passed" in result.output +def test_validate_reports_malformed_policy_without_traceback(tmp_path: Path) -> None: + policy = tmp_path / "broken-policy.yaml" + policy.write_text("severity_overrides: [", encoding="utf-8") + + result = CliRunner().invoke( + cli, + [ + "validate", + str(FIXTURE), + "--no-llm", + "--no-dedup", + "--checks", + "schema", + "--policy", + str(policy), + ], + ) + + assert result.exit_code == 1 + assert f"Invalid policy YAML in {policy}" in result.output + assert isinstance(result.exception, SystemExit) + + def test_validate_prints_tier1_section_banner() -> None: # The Tier 1 section is announced as it runs so it is visibly reported in # CI logs (SkillEvaluator parity), not only inside the final combined report. diff --git a/tests/validators/test_policy.py b/tests/validators/test_policy.py index ccfe701..f6bffba 100644 --- a/tests/validators/test_policy.py +++ b/tests/validators/test_policy.py @@ -8,8 +8,10 @@ from pathlib import Path import pytest +import yaml from skillevaluator.models.result import Severity +from skillevaluator.validators import policy as policy_module from skillevaluator.validators.policy import ( DEFAULT_PROFILE_NAME, ValidationPolicy, @@ -46,6 +48,75 @@ def test_custom_policy_overlays_the_public_profile(tmp_path: Path) -> None: assert policy.severity_for("SCHEMA", "author_missing", Severity.LOW) == Severity.HIGH +def test_custom_policy_wraps_malformed_yaml(tmp_path: Path) -> None: + custom = tmp_path / "broken-policy.yaml" + custom.write_text("severity_overrides: [", encoding="utf-8") + + with pytest.raises(ValueError) as exc_info: + load_policy_file(custom) + + assert f"Invalid policy YAML in {custom}" in str(exc_info.value) + assert isinstance(exc_info.value.__cause__, yaml.YAMLError) + + +def test_custom_policy_wraps_invalid_encoding(tmp_path: Path) -> None: + custom = tmp_path / "utf16-policy.yaml" + custom.write_bytes("profile: encoded\n".encode("utf-16")) + + with pytest.raises(ValueError) as exc_info: + load_policy_file(custom) + + assert f"Invalid policy YAML in {custom}" in str(exc_info.value) + assert isinstance(exc_info.value.__cause__, UnicodeDecodeError) + + +def test_custom_policy_wraps_read_errors(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + custom = tmp_path / "unreadable-policy.yaml" + custom.write_text("profile: unreadable\n", encoding="utf-8") + original_open = Path.open + + def deny_custom_policy(path: Path, *args: object, **kwargs: object): + if path == custom: + raise PermissionError("access denied") + return original_open(path, *args, **kwargs) + + monkeypatch.setattr(Path, "open", deny_custom_policy) + + with pytest.raises(ValueError) as exc_info: + load_policy_file(custom) + + assert f"Could not read policy file {custom}" in str(exc_info.value) + assert isinstance(exc_info.value.__cause__, PermissionError) + + +def test_policy_yaml_wraps_recursion_errors(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + policy = tmp_path / "deeply-nested-policy.yaml" + policy.write_text("severity_overrides: []\n", encoding="utf-8") + + def exhaust_parser(_stream: object) -> object: + raise RecursionError("maximum recursion depth exceeded") + + monkeypatch.setattr(policy_module.yaml, "safe_load", exhaust_parser) + + with pytest.raises(ValueError) as exc_info: + policy_module._load_policy_yaml(policy) + + assert f"Invalid policy YAML in {policy}" in str(exc_info.value) + assert isinstance(exc_info.value.__cause__, RecursionError) + + +def test_bundled_profile_wraps_malformed_yaml(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + profile = tmp_path / "broken.yaml" + profile.write_text("identity: [", encoding="utf-8") + monkeypatch.setattr(policy_module, "PROFILES_DIR", tmp_path) + + with pytest.raises(ValueError) as exc_info: + load_profile("broken") + + assert f"Invalid policy YAML in {profile}" in str(exc_info.value) + assert isinstance(exc_info.value.__cause__, yaml.YAMLError) + + def test_policy_validation_and_resolution() -> None: assert resolve_policy().profile == "external" assert (