From ae749bc94e93f21b428f656dad11c2d4aeaf21ac Mon Sep 17 00:00:00 2001 From: mimran-khan Date: Sat, 29 Aug 2026 03:38:04 +0530 Subject: [PATCH 1/4] feat(cli): write catalog-summary.json after catalog validate Emit a machine-readable fleet rollup at the reports root with per-skill status, optional severity totals from child JSON reports, and report paths. Create the output directory when needed so summary writes survive early skill failures. Fixes #120 Signed-off-by: mimran-khan --- CHANGELOG.md | 6 +++ src/skillevaluator/cli.py | 96 +++++++++++++++++++++++++++++++++++++++ tests/test_commands.py | 32 +++++++++---- 3 files changed, 125 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f54c4e84..8e7f6aa8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to SkillEvaluator are documented in this file. ## Unreleased +### 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. + ### Fixed - Tier 3 accuracy and custom goal judges now retry one malformed (including diff --git a/src/skillevaluator/cli.py b/src/skillevaluator/cli.py index 0ed29141..68de9590 100644 --- a/src/skillevaluator/cli.py +++ b/src/skillevaluator/cli.py @@ -6,8 +6,10 @@ from __future__ import annotations import copy +import json import logging import math +from datetime import UTC, datetime from pathlib import Path import click @@ -691,6 +693,89 @@ 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 _latest_skill_json_report(skill_report_dir: Path) -> Path | None: + """Return the newest per-skill machine-readable report when present.""" + if not skill_report_dir.is_dir(): + return None + candidates = sorted(skill_report_dir.glob("skillevaluator-output-*.json"), reverse=True) + if candidates: + return candidates[0] + return None + + +def _catalog_skill_entry( + skill_name: str, + skill_report_dir: Path, + *, + passed: bool, + reason: str, +) -> dict[str, object]: + entry: dict[str, object] = { + "name": skill_name, + "passed": passed, + "report_dir": skill_name, + } + if not passed: + entry["reason"] = reason + + json_report = _latest_skill_json_report(skill_report_dir) + if json_report is None: + 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.""" + 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 + output_path.write_text(json.dumps(summary, indent=2, default=str, allow_nan=False), encoding="utf-8") + return output_path + + def _validate_catalog( ctx: click.Context, *, @@ -725,6 +810,17 @@ def _validate_catalog( failures.append((skill_dir.name, str(getattr(exc, "message", exc)))) except Exception as exc: # unexpected: keep the catalog running, report it on the scoreboard failures.append((skill_dir.name, f"unexpected error: {exc}")) + 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, ""), + ) + 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( diff --git a/tests/test_commands.py b/tests/test_commands.py index b9e28b50..0b727bbe 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -326,6 +326,13 @@ 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_rejects_one_previous_version_for_every_skill() -> None: @@ -726,11 +733,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 +881,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: From ce97713af240468515ee40a5bcf675d77154e511 Mon Sep 17 00:00:00 2001 From: mimran-khan Date: Sat, 29 Aug 2026 03:45:05 +0530 Subject: [PATCH 2/4] feat(cli): add parallel catalog validation with --workers Catalog validate accepts --workers N to run skills in isolated child processes. Values above 1 skip the per-skill pipeline view and rebuild per-skill argv from the parent Click context or sys.argv. Fixes #122 Signed-off-by: mimran-khan --- CHANGELOG.md | 2 + src/skillevaluator/cli.py | 245 +++++++++++++++++++++++++++++++++- tests/golden/cli_surface.json | 18 +++ tests/test_commands.py | 38 ++++++ 4 files changed, 300 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e7f6aa8..ba71bd25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ All notable changes to SkillEvaluator are documented in this file. - 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). ### Fixed diff --git a/src/skillevaluator/cli.py b/src/skillevaluator/cli.py index 68de9590..e99ce3fb 100644 --- a/src/skillevaluator/cli.py +++ b/src/skillevaluator/cli.py @@ -9,8 +9,10 @@ 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 @@ -696,6 +698,164 @@ def _print_catalog_summary(total: int, failures: list[tuple[str, str]], reports_ 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 :] + if tail and not tail[0].startswith("-"): + tail = tail[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 + 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-skill", 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",): + if fmt == "cli": + continue + 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]: + """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"]) + result = CliRunner().invoke(cli, job["argv"]) + if result.exit_code == 0: + return skill_name, True, "" + exc = result.exception + if isinstance(exc, SystemExit): + return skill_name, False, "validation failed" + if exc is not None: + if isinstance(exc, click.ClickException): + return skill_name, False, str(getattr(exc, "message", exc)) + return skill_name, False, f"unexpected error: {exc}" + return skill_name, False, "validation failed" + + def _latest_skill_json_report(skill_report_dir: Path) -> Path | None: """Return the newest per-skill machine-readable report when present.""" if not skill_report_dir.is_dir(): @@ -781,12 +941,14 @@ def _validate_catalog( *, 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( @@ -795,6 +957,10 @@ 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]] = [] for index, skill_dir in enumerate(skill_dirs, start=1): _print_catalog_divider(index, len(skill_dirs), skill_dir.name) @@ -829,6 +995,67 @@ def _validate_catalog( ) +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 + + import sys + + parent_argv = list(sys.argv) + 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(ctx, skill_dir, output_dir / skill_dir.name, parent_argv), + "cwd": workdir, + } + for skill_dir in skill_dirs + ] + failures: list[tuple[str, str]] = [] + 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 = future.result() + 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, ""), + ) + 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 _rerun_hint(target_path: Path, agent_eval: bool) -> str: """Reconstruct the user's actual command for the FAIL panel's rerun line. @@ -1042,6 +1269,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", @@ -1245,6 +1482,7 @@ def validate( copy_repo: bool, timeout_multiplier: float | None, harbor_keep_jobs: bool, + workers: int, report_formats: tuple[str, ...], output_dir: Path, ) -> None: @@ -1315,7 +1553,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() @@ -1326,6 +1564,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 bc1aa820..4b770fb4 100644 --- a/tests/golden/cli_surface.json +++ b/tests/golden/cli_surface.json @@ -1512,6 +1512,15 @@ "param_type": "option", "type": "file" }, + { + "default": "1", + "name": "workers", + "opts": [ + "--workers" + ], + "param_type": "option", + "type": "integer range" + }, { "default": "True", "is_flag": true, @@ -2796,6 +2805,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 0b727bbe..6e38cdad 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -335,6 +335,44 @@ def test_validate_catalog_runs_each_skill_as_separate_job() -> None: 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", + "-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_rejects_one_previous_version_for_every_skill() -> None: runner = CliRunner() with runner.isolated_filesystem(): From 5a8230cd4daaee8fe80bdc013cc827747f7816db Mon Sep 17 00:00:00 2001 From: mimran-khan Date: Sat, 29 Aug 2026 17:20:30 +0530 Subject: [PATCH 3/4] fix(cli): stabilize parallel catalog workers and summary writes Rebuild child argv without dropping positional catalog paths, track fresh per-skill JSON reports instead of stale files, write catalog-summary.json atomically, and fix --include-skills forwarding for context fallback. Signed-off-by: mimran-khan --- src/skillevaluator/cli.py | 52 +++++++++++++++++++++++++++++---------- tests/test_commands.py | 27 ++++++++++++++++++++ 2 files changed, 66 insertions(+), 13 deletions(-) diff --git a/src/skillevaluator/cli.py b/src/skillevaluator/cli.py index e99ce3fb..2d03e5d3 100644 --- a/src/skillevaluator/cli.py +++ b/src/skillevaluator/cli.py @@ -707,8 +707,6 @@ def _catalog_child_argv_from_sys(skill_dir: Path, output_dir: Path, parent_argv: return ["validate", str(skill_dir), "-o", str(output_dir)] tail = argv[validate_idx + 1 :] - if tail and not tail[0].startswith("-"): - tail = tail[1:] child_tail: list[str] = [] skip_next = False @@ -721,6 +719,8 @@ def _catalog_child_argv_from_sys(skill_dir: Path, output_dir: Path, parent_argv: 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)] @@ -806,7 +806,7 @@ def _catalog_child_argv_from_ctx(ctx: click.Context, skill_dir: Path, output_dir if params.get("results_dir"): argv.extend(["--results-dir", str(params["results_dir"])]) for skill in params.get("include_skills") or (): - argv.extend(["--include-skill", str(skill)]) + argv.extend(["--include-skills", str(skill)]) if params.get("copy_repo"): argv.append("--copy-repo") if params.get("timeout_multiplier") is not None: @@ -832,7 +832,7 @@ def _catalog_child_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]: +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 @@ -843,17 +843,32 @@ def _run_catalog_skill_worker(job: dict[str, Any]) -> tuple[str, bool, str]: 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, "" + return skill_name, True, "", json_report_name exc = result.exception if isinstance(exc, SystemExit): - return skill_name, False, "validation failed" + 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)) - return skill_name, False, f"unexpected error: {exc}" - return skill_name, False, "validation failed" + 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 _latest_skill_json_report(skill_report_dir: Path) -> Path | None: @@ -872,6 +887,7 @@ def _catalog_skill_entry( *, passed: bool, reason: str, + json_report_name: str | None = None, ) -> dict[str, object]: entry: dict[str, object] = { "name": skill_name, @@ -881,8 +897,11 @@ def _catalog_skill_entry( if not passed: entry["reason"] = reason - json_report = _latest_skill_json_report(skill_report_dir) - if json_report is None: + if json_report_name: + json_report = skill_report_dir / json_report_name + else: + json_report = _latest_skill_json_report(skill_report_dir) + if json_report is None or not json_report.is_file(): return entry entry["json_report"] = json_report.name @@ -901,6 +920,8 @@ def _catalog_skill_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 @@ -932,7 +953,8 @@ def _write_catalog_summary(output_dir: Path, skills: list[dict[str, object]]) -> } output_dir.mkdir(parents=True, exist_ok=True) output_path = output_dir / CATALOG_SUMMARY_FILENAME - output_path.write_text(json.dumps(summary, indent=2, default=str, allow_nan=False), encoding="utf-8") + payload = json.dumps(summary, indent=2, default=str, allow_nan=False).encode("utf-8") + _write_report_atomically(output_path, payload) return output_path @@ -1025,14 +1047,17 @@ def _validate_catalog_parallel( "skill_name": skill_dir.name, "argv": _catalog_child_argv(ctx, skill_dir, output_dir / skill_dir.name, parent_argv), "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 = future.result() + skill_name, passed, reason, json_report_name = future.result() + worker_reports[skill_name] = json_report_name if not passed: failures.append((skill_name, reason)) @@ -1044,6 +1069,7 @@ def _validate_catalog_parallel( 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 ] diff --git a/tests/test_commands.py b/tests/test_commands.py index 6e38cdad..8fd517a0 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -373,6 +373,33 @@ def test_validate_catalog_workers_runs_skills_in_parallel() -> None: 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_rejects_one_previous_version_for_every_skill() -> None: runner = CliRunner() with runner.isolated_filesystem(): From 9330da5672ec2caeeb14abeb2f501fb2023e3d62 Mon Sep 17 00:00:00 2001 From: mimran-khan Date: Mon, 31 Aug 2026 22:24:30 +0530 Subject: [PATCH 4/4] fix(cli): address rng1995 parallel catalog worker review Rebuild child argv from Click params, preserve -r cli when selected, track per-run JSON in serial catalog mode, and stop attaching stale reports when no new JSON was produced this run. Signed-off-by: mimran-khan --- src/skillevaluator/cli.py | 41 +++++++++++---------- tests/test_commands.py | 75 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 21 deletions(-) diff --git a/src/skillevaluator/cli.py b/src/skillevaluator/cli.py index 2d03e5d3..cdf5df92 100644 --- a/src/skillevaluator/cli.py +++ b/src/skillevaluator/cli.py @@ -814,8 +814,6 @@ def _catalog_child_argv_from_ctx(ctx: click.Context, skill_dir: Path, output_dir if params.get("harbor_keep_jobs"): argv.append("--harbor-keep-jobs") for fmt in params.get("report_formats") or ("cli",): - if fmt == "cli": - continue argv.extend(["-r", fmt]) argv.extend(["-o", str(output_dir)]) return argv @@ -871,16 +869,6 @@ def _new_skill_json_report_name(output_dir: Path, existing_reports: set[Path]) - return sorted(new_reports, reverse=True)[0].name -def _latest_skill_json_report(skill_report_dir: Path) -> Path | None: - """Return the newest per-skill machine-readable report when present.""" - if not skill_report_dir.is_dir(): - return None - candidates = sorted(skill_report_dir.glob("skillevaluator-output-*.json"), reverse=True) - if candidates: - return candidates[0] - return None - - def _catalog_skill_entry( skill_name: str, skill_report_dir: Path, @@ -900,8 +888,9 @@ def _catalog_skill_entry( if json_report_name: json_report = skill_report_dir / json_report_name else: - json_report = _latest_skill_json_report(skill_report_dir) - if json_report is None or not json_report.is_file(): + return entry + + if not json_report.is_file(): return entry entry["json_report"] = json_report.name @@ -984,20 +973,32 @@ def _validate_catalog( 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( @@ -1005,6 +1006,7 @@ def _validate_catalog( 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 ] @@ -1032,9 +1034,6 @@ def _validate_catalog_parallel( _print_catalog_summary(0, [], output_dir) return - import sys - - parent_argv = list(sys.argv) workdir = str(Path.cwd()) output_dir.mkdir(parents=True, exist_ok=True) make_view_console().print( @@ -1045,7 +1044,7 @@ def _validate_catalog_parallel( jobs = [ { "skill_name": skill_dir.name, - "argv": _catalog_child_argv(ctx, skill_dir, output_dir / skill_dir.name, parent_argv), + "argv": _catalog_child_argv_from_ctx(ctx, skill_dir, output_dir / skill_dir.name), "cwd": workdir, "output_dir": str(output_dir / skill_dir.name), } diff --git a/tests/test_commands.py b/tests/test_commands.py index 8fd517a0..8635237c 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 @@ -357,6 +358,8 @@ def test_validate_catalog_workers_runs_skills_in_parallel() -> None: "--no-dedup", "--checks", "quality", + "-r", + "html", "-o", "out", ], @@ -400,6 +403,78 @@ def test_validate_catalog_workers_accepts_options_before_target_path() -> None: 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: runner = CliRunner() with runner.isolated_filesystem():