diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 78f312cf..39347e31 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,6 +20,7 @@ jobs: runs-on: ubuntu-latest outputs: docs_only: ${{ steps.changes.outputs.docs_only }} + metadata_only: ${{ steps.changes.outputs.metadata_only }} steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: @@ -37,6 +38,7 @@ jobs: if ! git show "$BASE_SHA:scripts/classify_ci_changes.py" > "$classifier"; then echo "::notice::Trusted classifier is not present on the base branch; running full CI." echo "docs_only=false" >> "$GITHUB_OUTPUT" + echo "metadata_only=false" >> "$GITHUB_OUTPUT" exit 0 fi python3 "$classifier" --base "$BASE_SHA" --head "$HEAD_SHA" @@ -204,7 +206,7 @@ jobs: tier3-macos: name: Tier 3 macOS contract and progress needs: classify-changes - if: ${{ !cancelled() && needs.classify-changes.outputs.docs_only != 'true' }} + if: ${{ !cancelled() && needs.classify-changes.outputs.docs_only != 'true' && needs.classify-changes.outputs.metadata_only != 'true' }} runs-on: macos-latest steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 6bf9e4f8..c7ec6d52 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -25,6 +25,7 @@ jobs: runs-on: ubuntu-latest outputs: docs_only: ${{ steps.changes.outputs.docs_only }} + metadata_only: ${{ steps.changes.outputs.metadata_only }} steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: @@ -42,6 +43,7 @@ jobs: if ! git show "$BASE_SHA:scripts/classify_ci_changes.py" > "$classifier"; then echo "::notice::Trusted classifier is not present on the base branch; running full CI." echo "docs_only=false" >> "$GITHUB_OUTPUT" + echo "metadata_only=false" >> "$GITHUB_OUTPUT" exit 0 fi python3 "$classifier" --base "$BASE_SHA" --head "$HEAD_SHA" diff --git a/CHANGELOG.md b/CHANGELOG.md index a2ad6515..910a62cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,13 @@ All notable changes to SkillEvaluator are documented in this file. ## Unreleased +### Added + +- `skillevaluator validate --tier3 --previous-skill ` now skips a fresh + Tier 3 run when only the `metadata` field in `SKILL.md` changed and the prior + skill has a generated `skill-card.md` or `BENCHMARK.md`. The decision fails + closed for any behavioral change, invalid frontmatter, or missing evidence. + ### Fixed - Tier 3 paired pass@k evidence now respects Python's active integer-string diff --git a/scripts/classify_ci_changes.py b/scripts/classify_ci_changes.py index aadb1f4b..8de27750 100644 --- a/scripts/classify_ci_changes.py +++ b/scripts/classify_ci_changes.py @@ -16,6 +16,8 @@ DOC_PREFIXES = (b"docs/", b"fern/") KNOWN_STATUSES = frozenset(b"ACDMRTUXB") +SKILL_FILENAME = b"SKILL.md" +TIER3_EVIDENCE_FILENAMES = (b"skill-card.md", b"BENCHMARK.md") def is_docs_only(paths: Sequence[bytes]) -> bool: @@ -23,6 +25,140 @@ def is_docs_only(paths: Sequence[bytes]) -> bool: return bool(paths) and all(path.startswith(DOC_PREFIXES) for path in paths) +def _is_skill_file(path: bytes) -> bool: + return path == SKILL_FILENAME or path.endswith(b"/" + SKILL_FILENAME) + + +def _split_frontmatter(content: bytes) -> tuple[bytes, bytes] | None: + """Split a Markdown file into YAML frontmatter and body, if present. + + This intentionally validates only the delimiter shape. The Tier 1 schema + validator remains responsible for validating the YAML itself; CI routing + must stay dependency-free because it runs before the project is installed. + """ + lines = content.splitlines(keepends=True) + if not lines or lines[0].rstrip(b"\r\n") != b"---": + return None + + for index, line in enumerate(lines[1:], start=1): + if line.rstrip(b"\r\n") in {b"---", b"..."}: + return b"".join(lines[: index + 1]), b"".join(lines[index + 1 :]) + return None + + +def _metadata_section(frontmatter: bytes) -> tuple[bytes, bytes, bytes] | None: + """Return the immutable prefix/suffix around a top-level ``metadata`` key. + + The PR classifier must run before dependencies are installed, so it keeps a + deliberately narrow YAML shape instead of parsing arbitrary YAML. Anything + outside this conventional top-level metadata block is treated as behavioral + and therefore falls back to a full Tier 3 run. + """ + lines = frontmatter.splitlines(keepends=True) + for index, line in enumerate(lines[1:-1], start=1): + if not line.startswith(b"metadata:"): + continue + value = line[len(b"metadata:") :].strip() + end = index + 1 + if not value or value.startswith(b"#"): + while end < len(lines) - 1: + candidate = lines[end] + if candidate.startswith((b" ", b"\t", b"\r", b"\n", b"#")): + end += 1 + continue + break + return b"".join(lines[:index]), b"".join(lines[index:end]), b"".join(lines[end:]) + return None + + +def _is_metadata_only_change(previous: bytes, current: bytes) -> bool: + previous_parts = _split_frontmatter(previous) + current_parts = _split_frontmatter(current) + if previous_parts is None or current_parts is None: + return False + previous_frontmatter, previous_body = previous_parts + current_frontmatter, current_body = current_parts + if previous_body != current_body: + return False + + previous_metadata = _metadata_section(previous_frontmatter) + current_metadata = _metadata_section(current_frontmatter) + if previous_metadata is None and current_metadata is None: + return False + if previous_metadata is None: + current_prefix, current_block, current_suffix = current_metadata + return current_prefix + current_suffix == previous_frontmatter and bool(current_block) + if current_metadata is None: + previous_prefix, previous_block, previous_suffix = previous_metadata + return previous_prefix + previous_suffix == current_frontmatter and bool(previous_block) + previous_prefix, previous_block, previous_suffix = previous_metadata + current_prefix, current_block, current_suffix = current_metadata + return ( + previous_prefix == current_prefix + and previous_suffix == current_suffix + and previous_block != current_block + ) + + +def _merge_base(repo: Path, base: str, head: str) -> str: + result = subprocess.run( + ["git", "-C", str(repo), "merge-base", base, head], + check=True, + capture_output=True, + ) + merge_base = result.stdout.strip().decode("ascii") + return _validate_revision(merge_base) + + +def _revision_file(repo: Path, revision: str, path: bytes) -> bytes: + """Read ``path`` from a Git revision without touching the worktree.""" + path_text = os.fsdecode(path) + result = subprocess.run( + ["git", "-C", str(repo), "show", f"{revision}:{path_text}"], + check=True, + capture_output=True, + ) + return result.stdout + + +def _has_tier3_evidence(repo: Path, revision: str, skill_path: bytes) -> bool: + skill_parent = skill_path.rsplit(b"/", 1)[0] if b"/" in skill_path else b"" + for filename in TIER3_EVIDENCE_FILENAMES: + evidence_path = filename if not skill_parent else skill_parent + b"/" + filename + try: + _revision_file(repo, revision, evidence_path) + except subprocess.CalledProcessError: + continue + return True + return False + + +def is_metadata_only(repo: Path, base: str, head: str, paths: Sequence[bytes]) -> bool: + """Return whether a diff changes only existing skills' frontmatter. + + Tier 3 is expensive and need not run after metadata-only edits, but this is + safe only when the affected skill already has a generated card or benchmark + from an earlier evaluation. Evidence is read from the merge-base revision + so a pull request cannot qualify itself by adding a new artifact. + """ + if not paths or not all(_is_skill_file(path) for path in paths): + return False + + merge_base = _merge_base(repo, base, head) + for path in paths: + if not _has_tier3_evidence(repo, merge_base, path): + return False + + try: + previous = _revision_file(repo, merge_base, path) + current = _revision_file(repo, head, path) + except subprocess.CalledProcessError: + return False + if not _is_metadata_only_change(previous, current): + return False + return True + + def parse_name_status_z(payload: bytes) -> list[bytes]: """Parse ``git diff --name-status -z`` without losing rename sources.""" if not payload: @@ -89,13 +225,16 @@ def changed_paths(repo: Path, base: str, head: str) -> list[bytes]: return parse_name_status_z(result.stdout) -def _write_result(docs_only: bool) -> None: - line = f"docs_only={'true' if docs_only else 'false'}" +def _write_result(docs_only: bool, metadata_only: bool) -> None: + lines = ( + f"docs_only={'true' if docs_only else 'false'}", + f"metadata_only={'true' if metadata_only else 'false'}", + ) output_path = os.environ.get("GITHUB_OUTPUT") if output_path: with Path(output_path).open("a", encoding="utf-8") as output: - output.write(f"{line}\n") - print(line) + output.writelines(f"{line}\n" for line in lines) + print(*lines, sep="\n") def _parser() -> argparse.ArgumentParser: @@ -114,11 +253,13 @@ def main(argv: Sequence[str] | None = None) -> int: if not paths: raise ValueError("no changed paths found") docs_only = is_docs_only(paths) + metadata_only = is_metadata_only(args.repo, args.base, args.head, paths) except (OSError, subprocess.CalledProcessError, ValueError) as error: print(f"change classification failed; falling back to full CI: {error}", file=sys.stderr) docs_only = False + metadata_only = False - _write_result(docs_only) + _write_result(docs_only, metadata_only) return 0 diff --git a/src/skillevaluator/cli.py b/src/skillevaluator/cli.py index 0ed29141..e3e3b09b 100644 --- a/src/skillevaluator/cli.py +++ b/src/skillevaluator/cli.py @@ -453,6 +453,7 @@ def _run_agent_eval_or_skip( harbor_keep_jobs: bool = False, block_on_agent_eval: bool = False, validate_source: bool = True, + previous_skill: Path | None = None, progress_reporter=None, ) -> ValidationResult: """Run Tier 3 live agent evaluation and fold the result into the combined report. @@ -462,6 +463,36 @@ def _run_agent_eval_or_skip( describing why Tier 3 could not run. Tier 3 remains advisory by default, and callers can opt into blocking behavior. """ + if previous_skill is not None: + from skillevaluator.evaluation.tier3_report import advisory_skip_result + from skillevaluator.tier3.change_detection import tier3_run_decision + + decision = tier3_run_decision(target_path, previous_skill) + if decision.should_skip: + change_summary = ( + "only SKILL.md metadata changed" + if decision.reason_code == "metadata_only_change" + else "SKILL.md content is unchanged" + ) + result = advisory_skip_result( + f"Tier 3 live evaluation skipped: {change_summary} " + f"since the prior evaluation ({decision.evidence_file}).", + skill_name=target_path.name, + ) + result.metadata["tier3_change_decision"] = decision.to_dict() + result.metadata["tier3_applicability"] = { + "applicability": "not_required", + "reason_code": decision.reason_code, + "source_kind": "skill", + } + payload = result.metadata.get("agent_eval") + if isinstance(payload, dict): + payload["reason_code"] = decision.reason_code + summary = payload.get("summary") + if isinstance(summary, dict): + summary["reason_code"] = decision.reason_code + return result + if validate_source: from skillevaluator.evaluation.tier3_report import dataset_required_result from skillevaluator.tier3.evals_spec import validate_tier3_source @@ -973,6 +1004,14 @@ def _print_run_banner(target_path: Path, content_type: str, profile: str | None) help_group=_TIER3_GROUP, help="Also run Tier 3 live agent evaluation (requires a valid eval dataset or native Harbor source).", ) +@click.option( + "--previous-skill", + type=click.Path(exists=True, path_type=Path), + default=None, + cls=GroupedOption, + help_group=_TIER3_GROUP, + help="Previous evaluated copy of this skill. Metadata-only SKILL.md changes skip a fresh Tier 3 run when it has a skill card or benchmark.", +) @click.option( "--block-on-agent-eval/--no-block-on-agent-eval", default=None, @@ -1131,6 +1170,7 @@ def validate( dedup: bool, block_on_dedup: bool | None, agent_eval: bool, + previous_skill: Path | None, block_on_agent_eval: bool | None, autopilot: bool, agents: str, @@ -1233,6 +1273,16 @@ def validate( ) return + tier3_change_decision = None + if previous_skill is not None: + if not agent_eval: + raise click.ClickException("--previous-skill requires --tier3 or --agent-eval.") + if not preflight_tier3_source: + raise click.ClickException("--previous-skill applies only to a single skill.") + from skillevaluator.tier3.change_detection import tier3_run_decision + + tier3_change_decision = tier3_run_decision(target_path, previous_skill) + # Quiet (default) drives the compact pipeline view; --verbose keeps the # historical full-detail stream, as does DEBUG logging via the group -v. quiet = not verbose and not logging.getLogger().isEnabledFor(logging.DEBUG) @@ -1351,7 +1401,7 @@ def _on_check(name: str) -> None: # Tier 3 is advisory, so a dataset-generation failure must not abort # validate after Tier 1/2 already ran -- Tier 3 skips with the reason. autopilot_error: str | None = None - if autopilot: + if autopilot and (tier3_change_decision is None or tier3_change_decision.should_run): try: dataset_note = _ensure_autopilot_dataset(target_path, quiet=quiet) except (Exception, SystemExit) as exc: @@ -1391,6 +1441,7 @@ def _on_engine_tail(lines: list[str]) -> None: harbor_keep_jobs=harbor_keep_jobs, block_on_agent_eval=block_on_agent_eval_effective, validate_source=preflight_tier3_source, + previous_skill=previous_skill, progress_reporter=reporter, ) results.append(tier3_result) @@ -1739,6 +1790,12 @@ def dedup_scan( show_default=True, help="Comma-separated Harbor agents (claude is an alias for claude-code).", ) +@click.option( + "--previous-skill", + type=click.Path(exists=True, path_type=Path), + default=None, + help="Previous evaluated copy of this skill. Metadata-only SKILL.md changes skip the live evaluation when it has a skill card or benchmark.", +) @click.option("--env-mode", default="docker", show_default=True, type=ENV_MODE_CHOICE) @click.option( "--autopilot", @@ -1783,6 +1840,7 @@ def dedup_scan( def evaluate( skill_path: Path, agents: str, + previous_skill: Path | None, env_mode: str, autopilot: bool, skip_baseline: bool, @@ -1808,6 +1866,17 @@ def evaluate( progress: str, ) -> None: """Run Tier 3 live agent evaluation.""" + if previous_skill is not None: + from skillevaluator.tier3.change_detection import tier3_run_decision + + decision = tier3_run_decision(skill_path, previous_skill) + if decision.should_skip: + click.echo( + "Tier 3 live evaluation skipped: " + f"{decision.reason_code} confirmed by prior {decision.evidence_file}." + ) + return + from skillevaluator.evaluation import EvaluationOptions, EvaluationService from skillevaluator.tier3.harbor.progress import create_progress_reporter diff --git a/src/skillevaluator/tier3/__init__.py b/src/skillevaluator/tier3/__init__.py index 9e254ce5..097b1e16 100644 --- a/src/skillevaluator/tier3/__init__.py +++ b/src/skillevaluator/tier3/__init__.py @@ -3,6 +3,7 @@ """Tier 3 synthetic dataset creation and live agent evaluation.""" +from skillevaluator.tier3.change_detection import Tier3RunDecision, tier3_run_decision from skillevaluator.tier3.commands import ( compare_results, create_dataset, @@ -13,10 +14,12 @@ ) __all__ = [ + "Tier3RunDecision", "compare_results", "create_dataset", "doctor", "evaluate", + "tier3_run_decision", "validate_evals", "view_results", ] diff --git a/src/skillevaluator/tier3/change_detection.py b/src/skillevaluator/tier3/change_detection.py new file mode 100644 index 00000000..a6bbc8de --- /dev/null +++ b/src/skillevaluator/tier3/change_detection.py @@ -0,0 +1,122 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Decide whether a skill change needs a fresh Tier 3 live evaluation.""" + +from __future__ import annotations + +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +import yaml + +SKILL_MANIFEST = "SKILL.md" +TIER3_EVIDENCE_FILES = ("skill-card.md", "BENCHMARK.md") + + +@dataclass(frozen=True) +class Tier3RunDecision: + """The fail-closed Tier 3 decision for one current/baseline skill pair.""" + + should_run: bool + reason_code: str + evidence_file: str | None = None + + @property + def should_skip(self) -> bool: + """Return whether a fresh Tier 3 live evaluation may be skipped.""" + return not self.should_run + + def to_dict(self) -> dict[str, str | bool | None]: + """Serialize the decision for CI logs and generated reports.""" + return { + "should_run": self.should_run, + "reason_code": self.reason_code, + "evidence_file": self.evidence_file, + } + + +def _skill_root(path: Path) -> Path: + return path.parent if path.name == SKILL_MANIFEST else path + + +def _read_manifest(skill_root: Path) -> bytes | None: + try: + return (skill_root / SKILL_MANIFEST).read_bytes() + except OSError: + return None + + +def _split_frontmatter(content: bytes) -> tuple[bytes, bytes] | None: + """Return frontmatter YAML and Markdown body without normalizing either.""" + lines = content.splitlines(keepends=True) + if not lines or lines[0].rstrip(b"\r\n") != b"---": + return None + for index, line in enumerate(lines[1:], start=1): + if line.rstrip(b"\r\n") == b"---": + return b"".join(lines[1:index]), b"".join(lines[index + 1 :]) + return None + + +def _frontmatter_mapping(frontmatter: bytes) -> dict[str, Any] | None: + try: + data = yaml.safe_load(frontmatter.decode("utf-8")) + except (UnicodeDecodeError, yaml.YAMLError): + return None + return data if isinstance(data, dict) else None + + +def _existing_evidence(skill_root: Path) -> str | None: + for filename in TIER3_EVIDENCE_FILES: + if (skill_root / filename).is_file(): + return filename + return None + + +def tier3_run_decision(skill_path: Path, previous_skill_path: Path) -> Tier3RunDecision: + """Return whether ``skill_path`` needs a new Tier 3 live evaluation. + + A fresh run is skipped only when the Markdown body and all frontmatter + fields except ``metadata`` are unchanged from ``previous_skill_path``. A + generated ``skill-card.md`` or ``BENCHMARK.md`` must already exist with the + previous skill. Any unreadable path, invalid frontmatter, missing evidence, + or behavioral change fails closed and requires Tier 3. + """ + skill_root = _skill_root(skill_path) + previous_root = _skill_root(previous_skill_path) + evidence_file = _existing_evidence(previous_root) + if evidence_file is None: + return Tier3RunDecision(True, "previous_tier3_evidence_missing") + + current_manifest = _read_manifest(skill_root) + previous_manifest = _read_manifest(previous_root) + if current_manifest is None or previous_manifest is None: + return Tier3RunDecision(True, "skill_manifest_unreadable", evidence_file) + + current_parts = _split_frontmatter(current_manifest) + previous_parts = _split_frontmatter(previous_manifest) + if current_parts is None or previous_parts is None: + return Tier3RunDecision(True, "skill_frontmatter_invalid", evidence_file) + + current_frontmatter, current_body = current_parts + previous_frontmatter, previous_body = previous_parts + if current_body != previous_body: + return Tier3RunDecision(True, "skill_body_changed", evidence_file) + + current_data = _frontmatter_mapping(current_frontmatter) + previous_data = _frontmatter_mapping(previous_frontmatter) + if current_data is None or previous_data is None: + return Tier3RunDecision(True, "skill_frontmatter_invalid", evidence_file) + + current_non_metadata = {key: value for key, value in current_data.items() if key != "metadata"} + previous_non_metadata = {key: value for key, value in previous_data.items() if key != "metadata"} + if current_non_metadata != previous_non_metadata: + return Tier3RunDecision(True, "skill_frontmatter_changed", evidence_file) + + if current_data.get("metadata") != previous_data.get("metadata"): + return Tier3RunDecision(False, "metadata_only_change", evidence_file) + return Tier3RunDecision(False, "skill_unchanged", evidence_file) + + +__all__ = ["Tier3RunDecision", "tier3_run_decision"] diff --git a/tests/golden/cli_surface.json b/tests/golden/cli_surface.json index bc1aa820..1da3cd8f 100644 --- a/tests/golden/cli_surface.json +++ b/tests/golden/cli_surface.json @@ -316,6 +316,14 @@ "param_type": "option", "type": "text" }, + { + "name": "previous_skill", + "opts": [ + "--previous-skill" + ], + "param_type": "option", + "type": "path" + }, { "choices": [ "docker", @@ -1550,6 +1558,14 @@ "param_type": "option", "type": "boolean" }, + { + "name": "previous_skill", + "opts": [ + "--previous-skill" + ], + "param_type": "option", + "type": "path" + }, { "is_flag": true, "name": "block_on_agent_eval", @@ -2202,6 +2218,14 @@ "param_type": "option", "type": "text" }, + { + "name": "previous_skill", + "opts": [ + "--previous-skill" + ], + "param_type": "option", + "type": "path" + }, { "choices": [ "docker", @@ -2834,6 +2858,14 @@ "param_type": "option", "type": "boolean" }, + { + "name": "previous_skill", + "opts": [ + "--previous-skill" + ], + "param_type": "option", + "type": "path" + }, { "is_flag": true, "name": "block_on_agent_eval", diff --git a/tests/test_ci_change_classifier.py b/tests/test_ci_change_classifier.py index fa2ee383..30b88bac 100644 --- a/tests/test_ci_change_classifier.py +++ b/tests/test_ci_change_classifier.py @@ -8,7 +8,7 @@ from pathlib import Path import pytest -from scripts.classify_ci_changes import changed_paths, is_docs_only, main, parse_name_status_z +from scripts.classify_ci_changes import changed_paths, is_docs_only, is_metadata_only, main, parse_name_status_z def _git(repo: Path, *args: str) -> str: @@ -27,6 +27,24 @@ def _commit(repo: Path, message: str) -> str: return _git(repo, "rev-parse", "HEAD") +def _write_skill(repo: Path, *, artifact: str | None = None, body: str = "Use the skill.\n") -> Path: + skill = repo / "skills" / "demo" + skill.mkdir(parents=True, exist_ok=True) + (skill / "SKILL.md").write_text( + "---\n" + "name: demo\n" + "description: Demo skill\n" + "metadata:\n" + " owner: examples\n" + "---\n" + f"{body}", + encoding="utf-8", + ) + if artifact: + (skill / artifact).write_text("# Existing Tier 3 evidence\n", encoding="utf-8") + return skill + + @pytest.fixture def git_repo(tmp_path: Path) -> tuple[Path, str]: _git(tmp_path, "init", "--quiet") @@ -124,8 +142,8 @@ def test_main_classifies_a_real_docs_only_diff( assert _classify(repo, base, head, output, monkeypatch) == 0 - assert capsys.readouterr().out == "docs_only=true\n" - assert output.read_text(encoding="utf-8") == "docs_only=true\n" + assert capsys.readouterr().out == "docs_only=true\nmetadata_only=false\n" + assert output.read_text(encoding="utf-8") == "docs_only=true\nmetadata_only=false\n" def test_main_classifies_a_real_mixed_diff_as_full_ci( @@ -142,8 +160,90 @@ def test_main_classifies_a_real_mixed_diff_as_full_ci( assert _classify(repo, base, head, output, monkeypatch) == 0 - assert capsys.readouterr().out == "docs_only=false\n" - assert output.read_text(encoding="utf-8") == "docs_only=false\n" + assert capsys.readouterr().out == "docs_only=false\nmetadata_only=false\n" + assert output.read_text(encoding="utf-8") == "docs_only=false\nmetadata_only=false\n" + + +@pytest.mark.parametrize("artifact", ["skill-card.md", "BENCHMARK.md"]) +def test_main_skips_tier3_for_existing_skill_metadata_only_changes( + git_repo: tuple[Path, str], + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + artifact: str, +) -> None: + repo, _ = git_repo + skill = _write_skill(repo, artifact=artifact) + base = _commit(repo, "add evaluated skill") + skill_file = skill / "SKILL.md" + skill_file.write_text( + skill_file.read_text(encoding="utf-8").replace("owner: examples", "owner: platform"), + encoding="utf-8", + ) + head = _commit(repo, "change skill metadata") + paths = changed_paths(repo, base, head) + output = tmp_path / "github-output" + + assert is_metadata_only(repo, base, head, paths) is True + assert _classify(repo, base, head, output, monkeypatch) == 0 + + assert capsys.readouterr().out == "docs_only=false\nmetadata_only=true\n" + assert output.read_text(encoding="utf-8") == "docs_only=false\nmetadata_only=true\n" + + +def test_metadata_only_requires_preexisting_tier3_evidence( + git_repo: tuple[Path, str], + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + repo, _ = git_repo + skill = _write_skill(repo) + base = _commit(repo, "add unevaluated skill") + skill_file = skill / "SKILL.md" + skill_file.write_text( + skill_file.read_text(encoding="utf-8").replace("owner: examples", "owner: platform"), + encoding="utf-8", + ) + (skill / "BENCHMARK.md").write_text("# Newly added evidence\n", encoding="utf-8") + head = _commit(repo, "add metadata and evidence") + paths = changed_paths(repo, base, head) + output = tmp_path / "github-output" + + # The head has a benchmark, but it was not present in the trusted baseline. + # It must not qualify the accompanying frontmatter edit for a Tier 3 skip. + assert is_metadata_only(repo, base, head, [b"skills/demo/SKILL.md"]) is False + assert is_metadata_only(repo, base, head, paths) is False + assert _classify(repo, base, head, output, monkeypatch) == 0 + assert output.read_text(encoding="utf-8") == "docs_only=false\nmetadata_only=false\n" + + +def test_metadata_only_rejects_a_skill_body_change( + git_repo: tuple[Path, str], +) -> None: + repo, _ = git_repo + skill = _write_skill(repo, artifact="BENCHMARK.md") + base = _commit(repo, "add evaluated skill") + skill_file = skill / "SKILL.md" + skill_file.write_text(skill_file.read_text(encoding="utf-8") + "Changed instruction.\n", encoding="utf-8") + head = _commit(repo, "change skill instructions") + + assert is_metadata_only(repo, base, head, changed_paths(repo, base, head)) is False + + +def test_metadata_only_rejects_a_non_metadata_frontmatter_change( + git_repo: tuple[Path, str], +) -> None: + repo, _ = git_repo + skill = _write_skill(repo, artifact="BENCHMARK.md") + base = _commit(repo, "add evaluated skill") + skill_file = skill / "SKILL.md" + skill_file.write_text( + skill_file.read_text(encoding="utf-8").replace("description: Demo skill", "description: Changed skill"), + encoding="utf-8", + ) + head = _commit(repo, "change skill description") + + assert is_metadata_only(repo, base, head, changed_paths(repo, base, head)) is False def test_main_treats_a_deleted_docs_file_as_docs_only( @@ -156,7 +256,7 @@ def test_main_treats_a_deleted_docs_file_as_docs_only( head = _commit(repo, "delete docs") assert _classify(repo, base, head, tmp_path / "github-output", monkeypatch) == 0 - assert (tmp_path / "github-output").read_text(encoding="utf-8") == "docs_only=true\n" + assert (tmp_path / "github-output").read_text(encoding="utf-8") == "docs_only=true\nmetadata_only=false\n" def test_main_checks_both_sides_of_a_rename( @@ -170,7 +270,7 @@ def test_main_checks_both_sides_of_a_rename( head = _commit(repo, "rename out of docs") assert _classify(repo, base, head, tmp_path / "github-output", monkeypatch) == 0 - assert (tmp_path / "github-output").read_text(encoding="utf-8") == "docs_only=false\n" + assert (tmp_path / "github-output").read_text(encoding="utf-8") == "docs_only=false\nmetadata_only=false\n" def test_main_treats_a_rename_within_docs_as_docs_only( @@ -183,7 +283,7 @@ def test_main_treats_a_rename_within_docs_as_docs_only( head = _commit(repo, "rename within docs") assert _classify(repo, base, head, tmp_path / "github-output", monkeypatch) == 0 - assert (tmp_path / "github-output").read_text(encoding="utf-8") == "docs_only=true\n" + assert (tmp_path / "github-output").read_text(encoding="utf-8") == "docs_only=true\nmetadata_only=false\n" def test_main_checks_the_source_of_a_rename_into_docs( @@ -196,7 +296,7 @@ def test_main_checks_the_source_of_a_rename_into_docs( head = _commit(repo, "rename into docs") assert _classify(repo, base, head, tmp_path / "github-output", monkeypatch) == 0 - assert (tmp_path / "github-output").read_text(encoding="utf-8") == "docs_only=false\n" + assert (tmp_path / "github-output").read_text(encoding="utf-8") == "docs_only=false\nmetadata_only=false\n" def test_changed_paths_detects_an_unmodified_copy_source_outside_docs( @@ -228,7 +328,7 @@ def test_main_uses_the_merge_base_when_the_base_branch_advances( advanced_base = _commit(repo, "advance base") assert _classify(repo, advanced_base, feature_head, tmp_path / "github-output", monkeypatch) == 0 - assert (tmp_path / "github-output").read_text(encoding="utf-8") == "docs_only=true\n" + assert (tmp_path / "github-output").read_text(encoding="utf-8") == "docs_only=true\nmetadata_only=false\n" @pytest.mark.parametrize("revision", ["0" * 40, "not-a-sha"]) @@ -245,9 +345,9 @@ def test_main_fails_closed_for_invalid_revisions( assert _classify(repo, revision, head, output, monkeypatch) == 0 captured = capsys.readouterr() - assert captured.out == "docs_only=false\n" + assert captured.out == "docs_only=false\nmetadata_only=false\n" assert "falling back to full CI" in captured.err - assert output.read_text(encoding="utf-8") == "docs_only=false\n" + assert output.read_text(encoding="utf-8") == "docs_only=false\nmetadata_only=false\n" def test_main_fails_closed_for_an_empty_diff( @@ -263,9 +363,9 @@ def test_main_fails_closed_for_an_empty_diff( assert _classify(repo, head, head, output, monkeypatch) == 0 captured = capsys.readouterr() - assert captured.out == "docs_only=false\n" + assert captured.out == "docs_only=false\nmetadata_only=false\n" assert "no changed paths" in captured.err - assert output.read_text(encoding="utf-8") == "existing=value\ndocs_only=false\n" + assert output.read_text(encoding="utf-8") == "existing=value\ndocs_only=false\nmetadata_only=false\n" def test_main_fails_closed_outside_a_git_repository( @@ -280,9 +380,9 @@ def test_main_fails_closed_outside_a_git_repository( assert _classify(tmp_path / "missing-repo", head, head, output, monkeypatch) == 0 captured = capsys.readouterr() - assert captured.out == "docs_only=false\n" + assert captured.out == "docs_only=false\nmetadata_only=false\n" assert "falling back to full CI" in captured.err - assert output.read_text(encoding="utf-8") == "docs_only=false\n" + assert output.read_text(encoding="utf-8") == "docs_only=false\nmetadata_only=false\n" def test_cli_classifies_a_real_fern_only_diff(git_repo: tuple[Path, str], tmp_path: Path) -> None: @@ -309,9 +409,9 @@ def test_cli_classifies_a_real_fern_only_diff(git_repo: tuple[Path, str], tmp_pa env={"GITHUB_OUTPUT": str(output)}, ) - assert result.stdout == "docs_only=true\n" + assert result.stdout == "docs_only=true\nmetadata_only=false\n" assert result.stderr == "" - assert output.read_text(encoding="utf-8") == "docs_only=true\n" + assert output.read_text(encoding="utf-8") == "docs_only=true\nmetadata_only=false\n" def test_output_write_failure_is_not_silently_downgraded( diff --git a/tests/test_ci_workflows.py b/tests/test_ci_workflows.py index e92a708d..78752f9f 100644 --- a/tests/test_ci_workflows.py +++ b/tests/test_ci_workflows.py @@ -25,6 +25,10 @@ HEAVY_CI_JOBS = set(REQUIRED_CI_JOBS) - {"test-python-312"} RUN_UNLESS_CANCELLED_IF = "${{ !cancelled() }}" FULL_LANE_IF = "${{ !cancelled() && needs.classify-changes.outputs.docs_only != 'true' }}" +TIER3_LANE_IF = ( + "${{ !cancelled() && needs.classify-changes.outputs.docs_only != 'true' && " + "needs.classify-changes.outputs.metadata_only != 'true' }}" +) DOCS_ONLY_IF = "${{ needs.classify-changes.outputs.docs_only == 'true' }}" NOT_DOCS_ONLY_IF = "${{ needs.classify-changes.outputs.docs_only != 'true' }}" PR_CONCURRENCY = { @@ -64,7 +68,7 @@ def test_ci_preserves_required_contexts_as_explicit_jobs() -> None: _assert_no_path_filter(ci) -def test_ci_classifier_is_pull_request_only_and_exports_docs_only() -> None: +def test_ci_classifier_is_pull_request_only_and_exports_routing_outputs() -> None: ci = _load("ci.yml") classifier = ci["jobs"]["classify-changes"] @@ -72,6 +76,7 @@ def test_ci_classifier_is_pull_request_only_and_exports_docs_only() -> None: assert classifier["name"] == "Classify changes" assert classifier["if"] == "${{ github.event_name == 'pull_request' }}" assert classifier["outputs"]["docs_only"] == "${{ steps.changes.outputs.docs_only }}" + assert classifier["outputs"]["metadata_only"] == "${{ steps.changes.outputs.metadata_only }}" assert classifier["steps"][0]["with"]["fetch-depth"] == "0" assert classifier["steps"][0]["with"]["persist-credentials"] == "false" assert classifier["steps"][1]["id"] == "changes" @@ -79,6 +84,7 @@ def test_ci_classifier_is_pull_request_only_and_exports_docs_only() -> None: assert 'git show "$BASE_SHA:scripts/classify_ci_changes.py"' in classifier_run assert 'python3 "$classifier"' in classifier_run assert 'echo "docs_only=false" >> "$GITHUB_OUTPUT"' in classifier_run + assert 'echo "metadata_only=false" >> "$GITHUB_OUTPUT"' in classifier_run assert "python3 scripts/classify_ci_changes.py" not in classifier_run assert classifier["steps"][1]["env"] == { "BASE_SHA": "${{ github.event.pull_request.base.sha }}", @@ -122,7 +128,8 @@ def test_ci_skips_every_other_required_job_only_after_classification() -> None: for job_id in HEAVY_CI_JOBS: assert jobs[job_id]["needs"] == "classify-changes" - assert jobs[job_id]["if"] == FULL_LANE_IF + expected_if = TIER3_LANE_IF if job_id == "tier3-macos" else FULL_LANE_IF + assert jobs[job_id]["if"] == expected_if def test_full_lane_keeps_the_existing_commands_and_runners() -> None: @@ -153,11 +160,13 @@ def test_security_keeps_gitleaks_always_on_and_skips_only_nonessential_jobs() -> assert "needs" not in jobs["gitleaks"] assert jobs["classify-changes"]["if"] == "${{ github.event_name == 'pull_request' }}" assert jobs["classify-changes"]["outputs"]["docs_only"] == "${{ steps.changes.outputs.docs_only }}" + assert jobs["classify-changes"]["outputs"]["metadata_only"] == "${{ steps.changes.outputs.metadata_only }}" assert jobs["classify-changes"]["steps"][0]["with"]["persist-credentials"] == "false" classifier_run = jobs["classify-changes"]["steps"][1]["run"] assert 'git show "$BASE_SHA:scripts/classify_ci_changes.py"' in classifier_run assert 'python3 "$classifier"' in classifier_run assert 'echo "docs_only=false" >> "$GITHUB_OUTPUT"' in classifier_run + assert 'echo "metadata_only=false" >> "$GITHUB_OUTPUT"' in classifier_run assert "python3 scripts/classify_ci_changes.py" not in classifier_run dependency_if = " ".join(jobs["dependency-review"]["if"].split()) diff --git a/tests/test_commands.py b/tests/test_commands.py index b9e28b50..09853cd0 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -579,6 +579,124 @@ def _normalized(*_args, **_kwargs) -> ValidationResult: assert (result.metadata or {}).get("execution_status") != "skipped" +def test_run_agent_eval_skips_metadata_only_change_before_starting_engine(monkeypatch, tmp_path) -> None: + from skillevaluator import cli as cli_module + from skillevaluator.evaluation.service import EvaluationService + + previous = tmp_path / "previous" + current = tmp_path / "current" + previous.mkdir() + current.mkdir() + (previous / "SKILL.md").write_text( + "---\nname: demo\ndescription: Demo\nmetadata:\n owner: platform\n---\n# Demo\n", + encoding="utf-8", + ) + (previous / "BENCHMARK.md").write_text("# Prior result\n", encoding="utf-8") + (current / "SKILL.md").write_text( + "---\nname: demo\ndescription: Demo\nmetadata:\n owner: release\n---\n# Demo\n", + encoding="utf-8", + ) + monkeypatch.setattr( + EvaluationService, + "evaluate", + lambda *_args, **_kwargs: pytest.fail("metadata-only changes must not start the live-eval engine"), + ) + + result = cli_module._run_agent_eval_or_skip( + current, + agents="codex", + env_mode="docker", + skip_baseline=False, + n_concurrent=None, + max_agents=None, + previous_skill=previous, + ) + + assert result.metadata["tier3_change_decision"] == { + "should_run": False, + "reason_code": "metadata_only_change", + "evidence_file": "BENCHMARK.md", + } + assert result.metadata["tier3_applicability"] == { + "applicability": "not_required", + "reason_code": "metadata_only_change", + "source_kind": "skill", + } + + +def test_validate_forwards_previous_skill_to_tier3(monkeypatch, tmp_path) -> None: + from skillevaluator import cli as cli_module + from skillevaluator.evaluation.tier3_report import advisory_skip_result + from skillevaluator.models.result import ValidationResult + + current = tmp_path / "current" / "demo" + previous = tmp_path / "previous" / "demo" + for skill, owner in ((current, "release"), (previous, "platform")): + skill.mkdir(parents=True) + (skill / "SKILL.md").write_text( + "---\nname: demo\ndescription: Demo\nmetadata:\n" + f" owner: {owner}\n---\n# Demo\n", + encoding="utf-8", + ) + (previous / "BENCHMARK.md").write_text("# Prior result\n", encoding="utf-8") + tier1 = ValidationResult(validator_name="SCHEMA") + tier1.add_success("schema", "ok") + captured: dict[str, Path | None] = {} + + def _tier3(*_args, **kwargs) -> ValidationResult: + captured["previous_skill"] = kwargs["previous_skill"] + return advisory_skip_result("metadata-only") + + monkeypatch.setattr(cli_module, "run_validation", lambda *_args, **_kwargs: [tier1]) + monkeypatch.setattr(cli_module, "_run_agent_eval_or_skip", _tier3) + monkeypatch.setattr(cli_module, "emit_reports", lambda *_args, **_kwargs: True) + + result = CliRunner().invoke( + cli, + [ + "validate", + str(current), + "--no-dedup", + "--tier3", + "--previous-skill", + str(previous), + "--checks", + "schema", + ], + ) + + assert result.exit_code == 0, result.output + assert captured["previous_skill"] == previous + + +def test_tier3_evaluate_skips_metadata_only_change_before_starting_engine(monkeypatch, tmp_path) -> None: + from skillevaluator.evaluation.service import EvaluationService + + previous = tmp_path / "previous" + current = tmp_path / "current" + for skill, owner in ((previous, "platform"), (current, "release")): + skill.mkdir() + (skill / "SKILL.md").write_text( + "---\nname: demo\ndescription: Demo\nmetadata:\n" + f" owner: {owner}\n---\n# Demo\n", + encoding="utf-8", + ) + (previous / "skill-card.md").write_text("# Prior result\n", encoding="utf-8") + monkeypatch.setattr( + EvaluationService, + "evaluate", + lambda *_args, **_kwargs: pytest.fail("metadata-only changes must not start the live-eval engine"), + ) + + result = CliRunner().invoke( + cli, + ["tier3", "evaluate", str(current), "--previous-skill", str(previous), "--progress", "off"], + ) + + assert result.exit_code == 0, result.output + assert "metadata_only_change" in result.output + + def test_validate_tier2_default_is_blocking_and_can_be_advisory(monkeypatch) -> None: from skillevaluator import cli as cli_module from skillevaluator.models.result import ValidationResult diff --git a/tests/test_tier3_change_detection.py b/tests/test_tier3_change_detection.py new file mode 100644 index 00000000..fccaef6f --- /dev/null +++ b/tests/test_tier3_change_detection.py @@ -0,0 +1,99 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from skillevaluator.tier3.change_detection import tier3_run_decision + + +def _write_skill( + root: Path, + *, + owner: str = "platform", + description: str = "Use the demo skill.", + body: str = "# Demo\n\nFollow the workflow.\n", + artifact: str | None = None, +) -> None: + root.mkdir() + (root / "SKILL.md").write_text( + "---\n" + "name: demo\n" + f"description: {description}\n" + "metadata:\n" + f" owner: {owner}\n" + "---\n" + f"{body}", + encoding="utf-8", + ) + if artifact is not None: + (root / artifact).write_text("# Prior Tier 3 result\n", encoding="utf-8") + + +@pytest.mark.parametrize("artifact", ["skill-card.md", "BENCHMARK.md"]) +def test_metadata_only_change_skips_tier3_with_prior_evidence(tmp_path: Path, artifact: str) -> None: + previous = tmp_path / "previous" + current = tmp_path / "current" + _write_skill(previous, artifact=artifact) + _write_skill(current, owner="release-engineering") + + decision = tier3_run_decision(current / "SKILL.md", previous) + + assert decision.should_skip is True + assert decision.reason_code == "metadata_only_change" + assert decision.evidence_file == artifact + assert decision.to_dict()["should_run"] is False + + +def test_metadata_only_change_requires_prior_tier3_evidence(tmp_path: Path) -> None: + previous = tmp_path / "previous" + current = tmp_path / "current" + _write_skill(previous) + _write_skill(current, owner="release-engineering", artifact="BENCHMARK.md") + + decision = tier3_run_decision(current, previous) + + assert decision.should_run is True + assert decision.reason_code == "previous_tier3_evidence_missing" + + +@pytest.mark.parametrize( + ("current_kwargs", "reason_code"), + [ + ({"body": "# Demo\n\nChanged workflow.\n"}, "skill_body_changed"), + ({"description": "Different behavior."}, "skill_frontmatter_changed"), + ], +) +def test_behavioral_skill_changes_require_tier3( + tmp_path: Path, + current_kwargs: dict[str, str], + reason_code: str, +) -> None: + previous = tmp_path / "previous" + current = tmp_path / "current" + _write_skill(previous, artifact="BENCHMARK.md") + _write_skill(current, **current_kwargs) + + decision = tier3_run_decision(current, previous) + + assert decision.should_run is True + assert decision.reason_code == reason_code + + +def test_invalid_frontmatter_requires_tier3(tmp_path: Path) -> None: + previous = tmp_path / "previous" + current = tmp_path / "current" + _write_skill(previous, artifact="skill-card.md") + current.mkdir() + (current / "SKILL.md").write_text( + "---\nmetadata: [\n---\n# Demo\n\nFollow the workflow.\n", + encoding="utf-8", + ) + + decision = tier3_run_decision(current, previous) + + assert decision.should_run is True + assert decision.reason_code == "skill_frontmatter_invalid"