diff --git a/CHANGELOG.md b/CHANGELOG.md index 57deee3..55f42a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,11 @@ All notable changes to SkillEvaluator are documented in this file. ### Added +- Catalog validation now writes `catalog-summary.json` at the reports root with + per-skill pass/fail status, optional severity rollups from child JSON reports, + and paths to per-skill report directories. +- Catalog `validate` accepts `--workers N` to validate skills in parallel child + processes (default 1 preserves the serial per-skill pipeline view). - SARIF 2.1.0 reporter (`-r sarif`) for GitHub Code Scanning and other SARIF consumers. Findings map to rule IDs, severity levels, and file locations from Tier 1 validation results. diff --git a/src/skillevaluator/cli.py b/src/skillevaluator/cli.py index 8aa423a..39ba465 100644 --- a/src/skillevaluator/cli.py +++ b/src/skillevaluator/cli.py @@ -6,9 +6,13 @@ from __future__ import annotations import copy +import json import logging import math +from concurrent.futures import ProcessPoolExecutor, as_completed +from datetime import UTC, datetime from pathlib import Path +from typing import Any import click @@ -692,17 +696,271 @@ def _print_catalog_summary(total: int, failures: list[tuple[str, str]], reports_ console_.print(Text.assemble((" reports ", MUTED), (f"{reports_root}//", MUTED))) +CATALOG_SUMMARY_FILENAME = "catalog-summary.json" + + +def _catalog_child_argv_from_sys(skill_dir: Path, output_dir: Path, parent_argv: list[str]) -> list[str]: + """Rebuild ``validate`` argv for one catalog skill from ``sys.argv``.""" + argv = list(parent_argv) + try: + validate_idx = next(i for i, arg in enumerate(argv) if arg == "validate") + except StopIteration: + return ["validate", str(skill_dir), "-o", str(output_dir)] + + tail = argv[validate_idx + 1 :] + + child_tail: list[str] = [] + skip_next = False + for arg in tail: + if skip_next: + skip_next = False + continue + if arg in {"--workers", "-o", "--output-dir"}: + skip_next = True + continue + if arg.startswith("--workers=") or arg.startswith("--output-dir=") or arg.startswith("-o="): + continue + if not arg.startswith("-"): + continue + child_tail.append(arg) + + return ["validate", str(skill_dir), *child_tail, "-o", str(output_dir)] + + +def _catalog_child_argv_from_ctx(ctx: click.Context, skill_dir: Path, output_dir: Path) -> list[str]: + """Rebuild ``validate`` argv from the active Click context (pytest-safe).""" + params = ctx.params + argv: list[str] = ["validate", str(skill_dir)] + + if params.get("verbose"): + argv.append("--verbose") + if params.get("full"): + argv.append("--full") + if params.get("tiers"): + argv.extend(["--tiers", str(params["tiers"])]) + if params.get("checks"): + argv.extend(["--checks", str(params["checks"])]) + if params.get("previous_version"): + argv.extend(["--previous-version", str(params["previous_version"])]) + if params.get("fail_fast"): + argv.append("--fail-fast") + if params.get("continue_on_failure"): + argv.append("-c") + if params.get("llm"): + argv.append("--llm") + else: + argv.append("--no-llm") + if params.get("llm_verify"): + argv.append("--llm-verify") + if not params.get("dedup", True): + argv.append("--no-dedup") + block_on_dedup = params.get("block_on_dedup") + if block_on_dedup is True: + argv.append("--block-on-dedup") + elif block_on_dedup is False: + argv.append("--no-block-on-dedup") + min_score = params.get("min_score", 70) + if min_score != 70: + argv.extend(["--min-score", str(min_score)]) + if params.get("external"): + argv.append("--external") + if params.get("policy_path"): + argv.extend(["--policy", str(params["policy_path"])]) + if params.get("profile"): + argv.extend(["--profile", str(params["profile"])]) + if params.get("agent_eval"): + argv.append("--tier3") + block_on_agent_eval = params.get("block_on_agent_eval") + if block_on_agent_eval is True: + argv.append("--block-on-agent-eval") + elif block_on_agent_eval is False: + argv.append("--no-block-on-agent-eval") + if params.get("autopilot"): + argv.append("--autopilot") + agents = params.get("agents", "codex") + if agents != "codex": + argv.extend(["--agents", str(agents)]) + env_mode = params.get("env_mode", "docker") + if env_mode != "docker": + argv.extend(["--env-mode", str(env_mode)]) + if params.get("skip_baseline"): + argv.append("--skip-baseline") + if params.get("n_concurrent") is not None: + argv.extend(["--n-concurrent", str(params["n_concurrent"])]) + if params.get("max_agents") is not None: + argv.extend(["--max-agents", str(params["max_agents"])]) + if params.get("n_attempts") is not None: + argv.extend(["--n-attempts", str(params["n_attempts"])]) + if params.get("pass_threshold") is not None: + argv.extend(["--pass-threshold", str(params["pass_threshold"])]) + stop_on_pass = params.get("stop_on_pass") + if stop_on_pass is True: + argv.append("--stop-on-pass") + elif stop_on_pass is False: + argv.append("--no-stop-on-pass") + if params.get("model"): + argv.extend(["--model", str(params["model"])]) + for override in params.get("agent_model") or (): + argv.extend(["--agent-model", str(override)]) + if params.get("grading_mode"): + argv.extend(["--grading-mode", str(params["grading_mode"])]) + if params.get("results_dir"): + argv.extend(["--results-dir", str(params["results_dir"])]) + for skill in params.get("include_skills") or (): + argv.extend(["--include-skills", str(skill)]) + if params.get("copy_repo"): + argv.append("--copy-repo") + if params.get("timeout_multiplier") is not None: + argv.extend(["--timeout-multiplier", str(params["timeout_multiplier"])]) + if params.get("harbor_keep_jobs"): + argv.append("--harbor-keep-jobs") + for fmt in params.get("report_formats") or ("cli",): + argv.extend(["-r", fmt]) + argv.extend(["-o", str(output_dir)]) + return argv + + +def _catalog_child_argv( + ctx: click.Context, + skill_dir: Path, + output_dir: Path, + parent_argv: list[str], +) -> list[str]: + if "validate" in parent_argv: + return _catalog_child_argv_from_sys(skill_dir, output_dir, parent_argv) + return _catalog_child_argv_from_ctx(ctx, skill_dir, output_dir) + + +def _run_catalog_skill_worker(job: dict[str, Any]) -> tuple[str, bool, str, str | None]: + """Run one catalog skill validation in a child process.""" + import os + + from click.testing import CliRunner + + workdir = job.get("cwd") + if workdir: + os.chdir(workdir) + + skill_name = str(job["skill_name"]) + output_dir = Path(job["output_dir"]) + existing_reports = ( + set(output_dir.glob("skillevaluator-output-*.json")) if output_dir.is_dir() else set() + ) + result = CliRunner().invoke(cli, job["argv"]) + json_report_name = _new_skill_json_report_name(output_dir, existing_reports) + if result.exit_code == 0: + return skill_name, True, "", json_report_name + exc = result.exception + if isinstance(exc, SystemExit): + return skill_name, False, "validation failed", json_report_name + if exc is not None: + if isinstance(exc, click.ClickException): + return skill_name, False, str(getattr(exc, "message", exc)), json_report_name + return skill_name, False, f"unexpected error: {exc}", json_report_name + return skill_name, False, "validation failed", json_report_name + + +def _new_skill_json_report_name(output_dir: Path, existing_reports: set[Path]) -> str | None: + """Return the JSON report filename produced during this worker run.""" + if not output_dir.is_dir(): + return None + new_reports = set(output_dir.glob("skillevaluator-output-*.json")) - existing_reports + if not new_reports: + return None + return sorted(new_reports, reverse=True)[0].name + + +def _catalog_skill_entry( + skill_name: str, + skill_report_dir: Path, + *, + passed: bool, + reason: str, + json_report_name: str | None = None, +) -> dict[str, object]: + entry: dict[str, object] = { + "name": skill_name, + "passed": passed, + "report_dir": skill_name, + } + if not passed: + entry["reason"] = reason + + if json_report_name: + json_report = skill_report_dir / json_report_name + else: + return entry + + if not json_report.is_file(): + return entry + + entry["json_report"] = json_report.name + try: + payload = json.loads(json_report.read_text(encoding="utf-8")) + except (json.JSONDecodeError, OSError): + return entry + if not isinstance(payload, dict): + return entry + + for key in ("overall_passed", "overall_status", "incomplete_scans", "severity_counts"): + if key in payload: + entry[key] = payload[key] + return entry + + +def _write_catalog_summary(output_dir: Path, skills: list[dict[str, object]]) -> Path: + """Write a machine-readable fleet rollup for catalog validation.""" + from skillevaluator.reporting.base import _write_report_atomically + + total = len(skills) + passed = sum(1 for skill in skills if skill.get("passed")) + failed = total - passed + severity_totals = { + "critical": 0, + "high": 0, + "medium": 0, + "low": 0, + } + for skill in skills: + counts = skill.get("severity_counts") + if not isinstance(counts, dict): + continue + for key in severity_totals: + value = counts.get(key) + if isinstance(value, int): + severity_totals[key] += value + + summary: dict[str, object] = { + "total": total, + "passed": passed, + "failed": failed, + "overall_passed": failed == 0, + "reports_root": output_dir.name, + "summary_path": CATALOG_SUMMARY_FILENAME, + "severity_totals": severity_totals, + "skills": skills, + "generated_at": datetime.now(tz=UTC).isoformat(), + } + output_dir.mkdir(parents=True, exist_ok=True) + output_path = output_dir / CATALOG_SUMMARY_FILENAME + payload = json.dumps(summary, indent=2, default=str, allow_nan=False).encode("utf-8") + _write_report_atomically(output_path, payload) + return output_path + + def _validate_catalog( ctx: click.Context, *, resolved_target: Path, output_dir: Path, + workers: int = 1, ) -> None: - """Run the full validate pipeline once per skill in the catalog, serially. + """Run the full validate pipeline once per skill in the catalog. Each skill is an independent job with its own pipeline view, reports (under ``//``), and verdict; the catalog exits nonzero - when any skill failed. + when any skill failed. With ``workers`` above 1, skills validate in + parallel child processes and the per-skill pipeline view is skipped. """ if ctx.params.get("previous_version"): raise click.ClickException( @@ -711,21 +969,111 @@ def _validate_catalog( ) skill_dirs = sorted(marker.parent for marker in resolved_target.glob("*/SKILL.md")) + if workers > 1: + _validate_catalog_parallel(ctx, skill_dirs=skill_dirs, output_dir=output_dir, workers=workers) + return + failures: list[tuple[str, str]] = [] + skill_reports: dict[str, str | None] = {} for index, skill_dir in enumerate(skill_dirs, start=1): _print_catalog_divider(index, len(skill_dirs), skill_dir.name) + skill_output = output_dir / skill_dir.name + existing_reports = ( + set(skill_output.glob("skillevaluator-output-*.json")) if skill_output.is_dir() else set() + ) overrides = { **ctx.params, "target_path": skill_dir, "content_type": "skill", - "output_dir": output_dir / skill_dir.name, + "output_dir": skill_output, } + passed = True + reason = "" try: ctx.invoke(validate, **overrides) except click.ClickException as exc: - failures.append((skill_dir.name, str(getattr(exc, "message", exc)))) + passed = False + reason = str(getattr(exc, "message", exc)) + failures.append((skill_dir.name, reason)) except Exception as exc: # unexpected: keep the catalog running, report it on the scoreboard - failures.append((skill_dir.name, f"unexpected error: {exc}")) + passed = False + reason = f"unexpected error: {exc}" + failures.append((skill_dir.name, reason)) + skill_reports[skill_dir.name] = _new_skill_json_report_name(skill_output, existing_reports) + failure_map = dict(failures) + skill_entries = [ + _catalog_skill_entry( + skill_dir.name, + output_dir / skill_dir.name, + passed=skill_dir.name not in failure_map, + reason=failure_map.get(skill_dir.name, ""), + json_report_name=skill_reports.get(skill_dir.name), + ) + for skill_dir in skill_dirs + ] + _write_catalog_summary(output_dir, skill_entries) + _print_catalog_summary(len(skill_dirs), failures, output_dir) + if failures: + raise click.ClickException( + f"{len(failures)}/{len(skill_dirs)} skills failed validation: " + + ", ".join(name for name, _reason in failures) + ) + + +def _validate_catalog_parallel( + ctx: click.Context, + *, + skill_dirs: list[Path], + output_dir: Path, + workers: int, +) -> None: + """Validate catalog skills concurrently in isolated child processes.""" + from skillevaluator.reporting.console_ui import make_view_console + + if not skill_dirs: + _write_catalog_summary(output_dir, []) + _print_catalog_summary(0, [], output_dir) + return + + workdir = str(Path.cwd()) + output_dir.mkdir(parents=True, exist_ok=True) + make_view_console().print( + f"[dim]Validating {len(skill_dirs)} skills with {workers} worker" + f"{'s' if workers != 1 else ''} (parallel catalog mode; per-skill pipeline view disabled)[/dim]" + ) + + jobs = [ + { + "skill_name": skill_dir.name, + "argv": _catalog_child_argv_from_ctx(ctx, skill_dir, output_dir / skill_dir.name), + "cwd": workdir, + "output_dir": str(output_dir / skill_dir.name), + } + for skill_dir in skill_dirs + ] + failures: list[tuple[str, str]] = [] + worker_reports: dict[str, str | None] = {} + with ProcessPoolExecutor(max_workers=workers) as executor: + futures = [executor.submit(_run_catalog_skill_worker, job) for job in jobs] + for future in as_completed(futures): + skill_name, passed, reason, json_report_name = future.result() + worker_reports[skill_name] = json_report_name + if not passed: + failures.append((skill_name, reason)) + + failures.sort(key=lambda item: item[0]) + failure_map = dict(failures) + skill_entries = [ + _catalog_skill_entry( + skill_dir.name, + output_dir / skill_dir.name, + passed=skill_dir.name not in failure_map, + reason=failure_map.get(skill_dir.name, ""), + json_report_name=worker_reports.get(skill_dir.name), + ) + for skill_dir in skill_dirs + ] + _write_catalog_summary(output_dir, skill_entries) _print_catalog_summary(len(skill_dirs), failures, output_dir) if failures: raise click.ClickException( @@ -947,6 +1295,16 @@ def _print_run_banner(target_path: Path, content_type: str, profile: str | None) help_group=_RUN_GROUP, help="Custom policy YAML overlaid on top of --profile.", ) +@click.option( + "--workers", + type=click.IntRange(1), + default=1, + show_default=True, + cls=GroupedOption, + help_group=_RUN_GROUP, + help="Concurrent catalog skill jobs when validating a folder of skills. " + "Values above 1 run skills in parallel processes and disable the per-skill pipeline view.", +) @click.option( "--dedup/--no-dedup", "--tier2/--no-tier2", @@ -1150,6 +1508,7 @@ def validate( copy_repo: bool, timeout_multiplier: float | None, harbor_keep_jobs: bool, + workers: int, report_formats: tuple[str, ...], output_dir: Path, ) -> None: @@ -1220,7 +1579,7 @@ def validate( from skillevaluator.constants import CONTENT_TYPE_UNKNOWN # A directory of skills (no root SKILL.md) is a catalog: run the pipeline - # once per skill, serially, each as its own job with its own reports. + # once per skill, each as its own job with its own reports. if ( resolved_type in (CONTENT_TYPE_SKILL, CONTENT_TYPE_UNKNOWN) and target_path.is_dir() @@ -1231,6 +1590,7 @@ def validate( click.get_current_context(), resolved_target=target_path, output_dir=output_dir, + workers=workers, ) return diff --git a/tests/golden/cli_surface.json b/tests/golden/cli_surface.json index a912b9a..0b59849 100644 --- a/tests/golden/cli_surface.json +++ b/tests/golden/cli_surface.json @@ -1525,6 +1525,15 @@ "param_type": "option", "type": "file" }, + { + "default": "1", + "name": "workers", + "opts": [ + "--workers" + ], + "param_type": "option", + "type": "integer range" + }, { "default": "True", "is_flag": true, @@ -2813,6 +2822,15 @@ "param_type": "option", "type": "file" }, + { + "default": "1", + "name": "workers", + "opts": [ + "--workers" + ], + "param_type": "option", + "type": "integer range" + }, { "default": "True", "is_flag": true, diff --git a/tests/test_commands.py b/tests/test_commands.py index b9e28b5..8635237 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -10,6 +10,7 @@ import pytest from click.testing import CliRunner +from skillevaluator import cli as cli_module from skillevaluator.cli import cli from skillevaluator.tier3.commands import parse_agent_model_overrides, parse_agents @@ -326,6 +327,152 @@ def test_validate_catalog_runs_each_skill_as_separate_job() -> None: assert result.exit_code == 0, result.output assert any(Path("out/simple").glob("*.html")) assert any(Path("out/simple2").glob("*.html")) + summary_path = Path("out/catalog-summary.json") + assert summary_path.is_file() + summary = json.loads(summary_path.read_text(encoding="utf-8")) + assert summary["total"] == 2 + assert summary["passed"] == 2 + assert summary["overall_passed"] is True + assert len(summary["skills"]) == 2 + + +def test_validate_catalog_workers_runs_skills_in_parallel() -> None: + runner = CliRunner() + with runner.isolated_filesystem(): + catalog = Path("catalog") + for name in ("simple", "simple2"): + shutil.copytree(FIXTURE, catalog / name) + second = catalog / "simple2" / "SKILL.md" + second.write_text( + second.read_text(encoding="utf-8").replace("name: simple", "name: simple2"), + encoding="utf-8", + ) + result = runner.invoke( + cli, + [ + "validate", + str(catalog.resolve()), + "--workers", + "2", + "--no-llm", + "--no-dedup", + "--checks", + "quality", + "-r", + "html", + "-o", + "out", + ], + ) + + out = _plain_text(result.output) + assert "parallel catalog mode" in out + assert "Catalog Result" in out + assert result.exit_code == 0, result.output + summary = json.loads(Path("out/catalog-summary.json").read_text(encoding="utf-8")) + assert summary["total"] == 2 + assert summary["passed"] == 2 + assert any(Path("out/simple").glob("*.html")) + assert any(Path("out/simple2").glob("*.html")) + + +def test_validate_catalog_workers_accepts_options_before_target_path() -> None: + runner = CliRunner() + with runner.isolated_filesystem(): + catalog = Path("catalog") + shutil.copytree(FIXTURE, catalog / "simple") + result = runner.invoke( + cli, + [ + "validate", + "--workers", + "2", + str(catalog.resolve()), + "--no-llm", + "--no-dedup", + "--checks", + "quality", + "-o", + "out", + ], + ) + + assert result.exit_code == 0, result.output + summary = json.loads(Path("out/catalog-summary.json").read_text(encoding="utf-8")) + assert summary["total"] == 1 + assert summary["passed"] == 1 + + +def test_validate_catalog_workers_preserves_min_score_and_json_format() -> None: + runner = CliRunner() + with runner.isolated_filesystem(): + catalog = Path("catalog") + shutil.copytree(FIXTURE, catalog / "simple") + result = runner.invoke( + cli, + [ + "validate", + str(catalog.resolve()), + "--workers", + "2", + "--no-llm", + "--no-dedup", + "--checks", + "quality", + "-r", + "json", + "-o", + "out", + ], + ) + assert result.exit_code == 0, result.output + assert any(Path("out/simple").glob("skillevaluator-output-*.json")) + + +def test_validate_catalog_workers_cli_only_report_format() -> None: + runner = CliRunner() + with runner.isolated_filesystem(): + catalog = Path("catalog") + shutil.copytree(FIXTURE, catalog / "simple") + result = runner.invoke( + cli, + [ + "validate", + str(catalog.resolve()), + "--workers", + "2", + "--no-llm", + "--no-dedup", + "--checks", + "quality", + "-r", + "cli", + "-o", + "out", + ], + ) + assert result.exit_code == 0, result.output + assert not any(Path("out/simple").glob("*.html")) + assert not any(Path("out/simple").glob("skillevaluator-output-*.json")) + + +def test_catalog_skill_entry_skips_stale_json_without_report_name(tmp_path: Path) -> None: + skill_dir = tmp_path / "simple" + skill_dir.mkdir() + stale = skill_dir / "skillevaluator-output-19990101T000000.json" + stale.write_text( + json.dumps({"overall_passed": True, "severity_counts": {"high": 7}}), + encoding="utf-8", + ) + entry = cli_module._catalog_skill_entry( + "simple", + skill_dir, + passed=False, + reason="validation failed", + json_report_name=None, + ) + assert "overall_passed" not in entry + assert "severity_counts" not in entry def test_validate_catalog_rejects_one_previous_version_for_every_skill() -> None: @@ -726,11 +873,15 @@ def _failing_tier1(*_args, **_kwargs) -> list[ValidationResult]: cli, ["validate", str(catalog.resolve()), "--no-llm", "--no-dedup", "--checks", "schema", "-o", "out"] ) - out = _plain_text(result.output) - assert "skill 1/2" in out and "skill 2/2" in out - assert "Catalog Result" in out - assert "0/2 skills passed" in out - assert result.exit_code != 0 + out = _plain_text(result.output) + assert "skill 1/2" in out and "skill 2/2" in out + assert "Catalog Result" in out + assert "0/2 skills passed" in out + assert result.exit_code != 0 + summary = json.loads(Path("out/catalog-summary.json").read_text(encoding="utf-8")) + assert summary["failed"] == 2 + assert summary["overall_passed"] is False + assert all(not skill["passed"] for skill in summary["skills"]) def test_render_evaluation_result_invokes_findings_report(monkeypatch) -> None: @@ -870,10 +1021,13 @@ def _boom(*_args, **_kwargs): cli, ["validate", str(catalog.resolve()), "--no-llm", "--no-dedup", "--checks", "schema", "-o", "out"] ) - out = _plain_text(result.output) - assert "Catalog Result" in out - assert "unexpected error: validator exploded" in out - assert result.exit_code != 0 + out = _plain_text(result.output) + assert "Catalog Result" in out + assert "unexpected error: validator exploded" in out + assert result.exit_code != 0 + summary = json.loads(Path("out/catalog-summary.json").read_text(encoding="utf-8")) + assert summary["failed"] == 2 + assert summary["skills"][0]["reason"] == "unexpected error: validator exploded" def test_summarize_tier2_empty_results_name_a_reason() -> None: