From bfdfc1a64eaf2634799b448b832b7b667950cfe3 Mon Sep 17 00:00:00 2001 From: vchamarthi <123653452+vchamarthi@users.noreply.github.com> Date: Thu, 22 Oct 2026 22:10:32 -0500 Subject: [PATCH 1/5] tools: validator checks, run_evals --skill, check_harbor_job --json, report-only fix --- .github/workflows/validate.yml | 21 ++ tools/check_harbor_job.py | 113 ++++-- tools/compare_harbor_skill.py | 2 +- tools/run_evals.py | 23 +- tools/tests/test_catalog_fields.py | 38 ++ tools/tests/test_check_harbor_job_json.py | 328 ++++++++++++++++++ .../tests/test_compare_harbor_report_only.py | 64 ++++ tools/tests/test_run_evals_skill_filter.py | 41 +++ tools/validate_skills.py | 91 +++++ 9 files changed, 685 insertions(+), 36 deletions(-) create mode 100644 tools/tests/test_catalog_fields.py create mode 100644 tools/tests/test_check_harbor_job_json.py create mode 100644 tools/tests/test_compare_harbor_report_only.py create mode 100644 tools/tests/test_run_evals_skill_filter.py diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 367d77c..47b0d84 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -46,6 +46,24 @@ jobs: - name: Eval schema run: python3 tools/run_evals.py --validate + # Standard library only. Counting before running matters: unittest reports a + # directory with no collected tests as a pass, so a rename that stops discovery + # would read exactly like a green suite. + - name: Tool tests + run: | + python3 - <<'PY' + import sys + import unittest + + suite = unittest.defaultTestLoader.discover("tools/tests", top_level_dir="tools/tests") + count = suite.countTestCases() + if count == 0: + sys.exit("no tests collected under tools/tests") + print(f"collected {count} tests") + result = unittest.TextTestRunner(verbosity=2).run(suite) + sys.exit(0 if result.wasSuccessful() else 1) + PY + # First, because a gate is worth what the detector behind it is worth and every way # this one can break is silent: a regex that stops matching reports a clean zero for # every task, which reads exactly like a repository whose tasks are all sound. So it @@ -79,6 +97,9 @@ jobs: - name: Overlap detector still detects run: python3 tools/lint_skill_overlap.py --self-test + - name: skills.sh schema field types are checked + run: python3 tools/validate_skills.py --self-test + # Advisory: which of two overlapping skills should win is a judgement a reviewer makes # with the contributor. One finding blocks anyway - containment 1.0 on a pair with a # skill authored here does nothing the other does not, so there is no division of diff --git a/tools/check_harbor_job.py b/tools/check_harbor_job.py index b5e4f7b..ed6a88d 100644 --- a/tools/check_harbor_job.py +++ b/tools/check_harbor_job.py @@ -30,20 +30,45 @@ import argparse import json +import math import sys from pathlib import Path REWARD_KEY = "reward" +class LoadError(Exception): + """A job or trial result.json that cannot be read.""" + + def load(path: Path) -> dict: try: with path.open(encoding="utf-8") as handle: - return json.load(handle) + data = json.load(handle) except FileNotFoundError: - sys.exit(f"FAIL no such file: {path}") + raise LoadError(f"no such file: {path}") from None except json.JSONDecodeError as exc: - sys.exit(f"FAIL {path} is not valid JSON: {exc}") + raise LoadError(f"{path} is not valid JSON: {exc}") from None + except (OSError, UnicodeDecodeError) as exc: + raise LoadError(f"cannot read {path}: {exc}") from None + except RecursionError: + raise LoadError(f"{path} is nested too deeply to parse") from None + except ValueError as exc: + # The decoder's integer digit limit raises a plain ValueError. + raise LoadError(f"{path} cannot be parsed: {exc}") from None + if not isinstance(data, dict): + raise LoadError(f"{path} is not a JSON object") + return data + + +def as_dict(value: object) -> dict: + """`value` when it is a JSON object, else an empty one (wrong-shaped field).""" + return value if isinstance(value, dict) else {} + + +def label(value: object) -> str | None: + """`value` when it is a non-empty string, else None (wrong-shaped name).""" + return value if isinstance(value, str) and value else None def collect_trials(job_result: Path) -> list[dict]: @@ -55,14 +80,31 @@ def collect_trials(job_result: Path) -> list[dict]: trials = [] for candidate in sorted(job_result.parent.glob("*/result.json")): trials.append(load(candidate)) - return sorted(trials, key=lambda t: (t.get("task_name") or "", t.get("trial_name") or "")) + return sorted( + trials, + key=lambda t: (label(t.get("task_name")) or "", label(t.get("trial_name")) or ""), + ) + + +def rewards_of(trial: dict) -> dict: + return as_dict(as_dict(trial.get("verifier_result")).get("rewards")) def reward_of(trial: dict) -> float | None: - """The trial's combined reward, or None when the verifier produced none.""" - rewards = (trial.get("verifier_result") or {}).get("rewards") or {} - value = rewards.get(REWARD_KEY) - return float(value) if isinstance(value, (int, float)) else None + """The trial's combined reward, or None when the verifier produced none. + + NaN and infinity are treated as no reward: they have no meaningful floor + comparison and are not representable in standard JSON output. So is an + integer too large to convert to a float. + """ + value = rewards_of(trial).get(REWARD_KEY) + if not isinstance(value, (int, float)): + return None + try: + number = float(value) + except OverflowError: + return None + return number if math.isfinite(number) else None def main() -> int: @@ -85,11 +127,24 @@ def main() -> int: action="store_true", help="report errored trials without failing; for exploratory runs only", ) + parser.add_argument( + "--json", + action="store_true", + help="print one JSON verdict (ok, problems, trials) instead of the text report", + ) args = parser.parse_args() + # In --json mode the text report is dropped; stdout carries only the verdict. + say = (lambda *a, **k: None) if args.json else print - job = load(args.result) - trials = collect_trials(args.result) - stats = job.get("stats") or {} + try: + job = load(args.result) + trials = collect_trials(args.result) + except LoadError as exc: + if args.json: + print(json.dumps({"ok": False, "problems": [str(exc)], "trials": []}, indent=2)) + return 1 + sys.exit(f"FAIL {exc}") + stats = as_dict(job.get("stats")) problems: list[str] = [] n_reported = job.get("n_total_trials") @@ -103,31 +158,34 @@ def main() -> int: f"expected {args.expected_trials} trial directories, found {len(trials)}" ) - print(f"job {args.result.parent.name}") - print( + say(f"job {args.result.parent.name}") + say( f"trials {stats.get('n_completed_trials')} completed, " f"{stats.get('n_errored_trials')} errored, " f"{stats.get('n_cancelled_trials')} cancelled, " f"{stats.get('n_retries')} retries" ) - print(f"floor reward >= {args.reward_floor}") - print() + say(f"floor reward >= {args.reward_floor}") + say() + rows: list[dict] = [] for trial in trials: - name = trial.get("task_name") or trial.get("trial_name") or "" + name = label(trial.get("task_name")) or label(trial.get("trial_name")) or "" exception = trial.get("exception_info") reward = reward_of(trial) + # Any truthy exception_info marks an error, even a wrong-shaped one. + kind = (label(as_dict(exception).get("exception_type")) or "unknown") if exception else None + rows.append({"task": name, "reward": reward, "error": kind}) if exception: - kind = exception.get("exception_type") or "unknown" line = f"ERROR {name}: {kind}" - print(line) + say(line) if not args.allow_errors: problems.append(f"{name} errored: {kind}") continue if reward is None: - print(f"ERROR {name}: verifier reported no '{REWARD_KEY}'") + say(f"ERROR {name}: verifier reported no '{REWARD_KEY}'") problems.append( f"{name} produced no reward. The verifier must write " f"/logs/verifier/reward.txt or reward.json." @@ -135,7 +193,7 @@ def main() -> int: continue verdict = "ok " if reward >= args.reward_floor else "BELOW" - print(f"{verdict} {name}: {reward:.3f}") + say(f"{verdict} {name}: {reward:.3f}") if reward < args.reward_floor: problems.append(f"{name} scored {reward:.3f} < {args.reward_floor}") @@ -143,21 +201,24 @@ def main() -> int: # them so a component regression is visible even when the total passes. extra = { key: value - for key, value in ((trial.get("verifier_result") or {}).get("rewards") or {}).items() + for key, value in rewards_of(trial).items() if key != REWARD_KEY } if extra: parts = ", ".join(f"{key}={value}" for key, value in sorted(extra.items())) - print(f" components: {parts}") + say(f" components: {parts}") - print() + say() + if args.json: + print(json.dumps({"ok": not problems, "problems": problems, "trials": rows}, indent=2)) + return 1 if problems else 0 if problems: - print(f"FAIL {len(problems)} problem(s):") + say(f"FAIL {len(problems)} problem(s):") for problem in problems: - print(f" - {problem}") + say(f" - {problem}") return 1 - print(f"OK {len(trials)} trial(s) at or above {args.reward_floor}") + say(f"OK {len(trials)} trial(s) at or above {args.reward_floor}") return 0 diff --git a/tools/compare_harbor_skill.py b/tools/compare_harbor_skill.py index 2e6b6a6..0a36a5b 100644 --- a/tools/compare_harbor_skill.py +++ b/tools/compare_harbor_skill.py @@ -462,7 +462,7 @@ def format_report(args: argparse.Namespace, tasks: list[str], results: dict, pro value = results[arm]["per_task"].get(task) cells.append("—" if value is None else f"{value:.3f}") baseline = results[BASELINE]["per_task"].get(task) - candidate = results["candidate"]["per_task"].get(task) + candidate = results.get("candidate", {}).get("per_task", {}).get(task) if baseline is None or candidate is None: delta = "—" else: diff --git a/tools/run_evals.py b/tools/run_evals.py index c1acc02..fea4fd0 100644 --- a/tools/run_evals.py +++ b/tools/run_evals.py @@ -44,8 +44,10 @@ def normalize(text: str) -> str: return re.sub(r"\s+", " ", text.casefold()) -def eval_files() -> list[Path]: - return sorted(SKILLS_DIR.glob("*/evals/evals.json")) +def eval_files(skill: str | None = None) -> list[Path]: + """Every skills/*/evals/evals.json, or just the one for `skill`.""" + pattern = f"{skill}/evals/evals.json" if skill else "*/evals/evals.json" + return sorted(SKILLS_DIR.glob(pattern)) def validate_file(path: Path) -> list[str]: @@ -135,14 +137,14 @@ def validate_file(path: Path) -> list[str]: return errors -def cmd_validate() -> int: +def cmd_validate(skill: str | None = None) -> int: if not SCHEMA_PATH.is_file(): print(f"FAIL missing schema: {SCHEMA_PATH}", file=sys.stderr) return 1 - files = eval_files() + files = eval_files(skill) if not files: - print("FAIL no skills/*/evals/evals.json found", file=sys.stderr) + print(f"FAIL no skills/{skill or '*'}/evals/evals.json found", file=sys.stderr) return 1 all_errors: list[str] = [] @@ -163,7 +165,7 @@ def cmd_validate() -> int: return 0 -def cmd_score(answers_path: Path, fail_under: float) -> int: +def cmd_score(answers_path: Path, fail_under: float, skill: str | None = None) -> int: answers = json.loads(answers_path.read_text(encoding="utf-8")) if not isinstance(answers, dict): print("FAIL answers file must be an object of {eval-id: answer}", file=sys.stderr) @@ -173,7 +175,7 @@ def cmd_score(answers_path: Path, fail_under: float) -> int: total = 0 missing: list[str] = [] - for path in eval_files(): + for path in eval_files(skill): data = json.loads(path.read_text(encoding="utf-8")) for case in data["evals"]: total += 1 @@ -227,12 +229,15 @@ def main() -> int: default=1.0, help="minimum pass rate when scoring (default: 1.0)", ) + parser.add_argument( + "--skill", help="limit --validate or --answers to skills//evals/evals.json" + ) args = parser.parse_args() if args.validate: - return cmd_validate() + return cmd_validate(args.skill) if args.answers: - return cmd_score(args.answers, args.fail_under) + return cmd_score(args.answers, args.fail_under, args.skill) parser.error("pass --validate or --answers FILE") return 2 diff --git a/tools/tests/test_catalog_fields.py b/tools/tests/test_catalog_fields.py new file mode 100644 index 0000000..93b83c7 --- /dev/null +++ b/tools/tests/test_catalog_fields.py @@ -0,0 +1,38 @@ +"""check_catalog rejects a skills.yaml key nothing reads and an unknown intel-hw-class.""" + +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +import validate_skills as v # noqa: E402 + + +def errors_for(entry: dict[str, str]) -> list[str]: + report = v.Report() + entry = {"name": "dpnp-quickstart", "maintainer": "someone", **entry} + v.check_catalog({"dpnp-quickstart": entry}, {"dpnp-quickstart": "tool-skill"}, report) + return report.errors + + +class CatalogFields(unittest.TestCase): + def test_known_fields_pass(self) -> None: + self.assertEqual(errors_for({"intel-hw-class": "gpu", "intel-products": "dpnp"}), []) + + def test_empty_hw_class_means_not_applicable(self) -> None: + self.assertEqual(errors_for({"intel-hw-class": ""}), []) + + def test_misspelt_key_is_an_error(self) -> None: + errors = errors_for({"intel-hw-clas": "gpu"}) + self.assertEqual(len(errors), 1) + self.assertIn("unknown key 'intel-hw-clas'", errors[0]) + + def test_unknown_hw_class_is_an_error(self) -> None: + errors = errors_for({"intel-hw-class": "GPU"}) + self.assertEqual(len(errors), 1) + self.assertIn("intel-hw-class must be one of", errors[0]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tools/tests/test_check_harbor_job_json.py b/tools/tests/test_check_harbor_job_json.py new file mode 100644 index 0000000..c96d30f --- /dev/null +++ b/tools/tests/test_check_harbor_job_json.py @@ -0,0 +1,328 @@ +"""check_harbor_job.py --json prints one machine-readable verdict.""" + +from __future__ import annotations + +import json +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +TOOL = Path(__file__).resolve().parents[1] / "check_harbor_job.py" + + +def make_job(root: Path, rewards: dict[str, float | None]) -> Path: + job = root / "job" + job.mkdir() + (job / "result.json").write_text( + json.dumps({"n_total_trials": len(rewards), "stats": {}}), encoding="utf-8" + ) + for name, reward in rewards.items(): + trial = job / f"{name}__abc" + trial.mkdir() + body = {"task_name": name, "trial_name": f"{name}__abc"} + if reward is not None: + body["verifier_result"] = {"rewards": {"reward": reward}} + (trial / "result.json").write_text(json.dumps(body), encoding="utf-8") + return job / "result.json" + + +def run(result: Path, *extra: str) -> subprocess.CompletedProcess: + return subprocess.run( + [sys.executable, str(TOOL), str(result), "--expected-trials", "2", *extra], + capture_output=True, + text=True, + ) + + +def strict_loads(text: str): + def reject(token: str): + raise ValueError(f"non-standard JSON token {token}") + + return json.loads(text, parse_constant=reject) + + +def write_trial_text(result: Path, text: str) -> None: + (result.parent / "b__abc" / "result.json").write_text(text, encoding="utf-8") + + +class CheckHarborJobJsonTest(unittest.TestCase): + def setUp(self): + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + self.tmp = Path(tmp.name) + + def subdir(self, name: str) -> Path: + root = self.tmp / name + root.mkdir() + return root + + def assert_structured_failure(self, proc, fragment: str): + self.assertEqual(proc.returncode, 1) + self.assertNotIn("Traceback", proc.stderr) + data = strict_loads(proc.stdout) + self.assertIs(data["ok"], False) + self.assertTrue(any(fragment in p for p in data["problems"]), data["problems"]) + return data + + def test_json_pass(self): + proc = run(make_job(self.tmp, {"a": 1.0, "b": 1.0}), "--json") + self.assertEqual(proc.returncode, 0) + data = json.loads(proc.stdout) + self.assertIs(data["ok"], True) + self.assertEqual(data["problems"], []) + self.assertEqual([t["task"] for t in data["trials"]], ["a", "b"]) + self.assertEqual(data["trials"][0]["reward"], 1.0) + + def test_json_fail_lists_problem(self): + proc = run(make_job(self.tmp, {"a": 1.0, "b": 0.5}), "--json") + self.assertEqual(proc.returncode, 1) + data = json.loads(proc.stdout) + self.assertIs(data["ok"], False) + self.assertTrue(any("b scored" in p for p in data["problems"])) + + def test_json_missing_reward_is_null(self): + proc = run(make_job(self.tmp, {"a": 1.0, "b": None}), "--json") + self.assertEqual(proc.returncode, 1) + data = json.loads(proc.stdout) + self.assertIsNone(data["trials"][1]["reward"]) + + def test_json_missing_job_is_structured_failure(self): + proc = run(self.tmp / "missing.json", "--json") + self.assertEqual(proc.returncode, 1) + data = strict_loads(proc.stdout) + self.assertIs(data["ok"], False) + self.assertTrue(any("no such file" in p for p in data["problems"])) + self.assertEqual(data["trials"], []) + + def test_json_malformed_job_is_structured_failure(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + result.write_text("{not json", encoding="utf-8") + proc = run(result, "--json") + self.assertEqual(proc.returncode, 1) + data = strict_loads(proc.stdout) + self.assertIs(data["ok"], False) + self.assertTrue(any("not valid JSON" in p for p in data["problems"])) + + def test_json_malformed_trial_is_structured_failure(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + write_trial_text(result, "{") + proc = run(result, "--json") + self.assertEqual(proc.returncode, 1) + self.assertIs(strict_loads(proc.stdout)["ok"], False) + + def test_json_non_object_job_is_structured_failure(self): + for index, body in enumerate(("[]", "null", "3")): + with self.subTest(body=body): + result = make_job(self.subdir(f"job-{index}"), {"a": 1.0, "b": 1.0}) + result.write_text(body, encoding="utf-8") + self.assert_structured_failure(run(result, "--json"), "not a JSON object") + + def test_json_non_object_trial_is_structured_failure(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + write_trial_text(result, "[1]") + self.assert_structured_failure(run(result, "--json"), "not a JSON object") + + def test_json_directory_as_job_file_is_structured_failure(self): + result = self.tmp / "result.json" + result.mkdir() + self.assert_structured_failure(run(result, "--json"), "cannot read") + + def test_json_directory_as_trial_file_is_structured_failure(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + trial_file = result.parent / "b__abc" / "result.json" + trial_file.unlink() + trial_file.mkdir() + self.assert_structured_failure(run(result, "--json"), "cannot read") + + def test_json_invalid_utf8_is_structured_failure(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + result.write_bytes(b'{"n_total_trials": "\xff\xfe"}') + self.assert_structured_failure(run(result, "--json"), "cannot read") + + def test_text_mode_non_object_job_fails_without_traceback(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + result.write_text("[]", encoding="utf-8") + proc = run(result) + self.assertEqual(proc.returncode, 1) + self.assertEqual(proc.stdout, "") + self.assertIn("FAIL", proc.stderr) + self.assertNotIn("Traceback", proc.stderr) + + def test_json_wrong_shaped_job_stats_does_not_crash(self): + for index, stats in enumerate(("[]", '"x"', "7", "null")): + with self.subTest(stats=stats): + result = make_job(self.subdir(f"stats-{index}"), {"a": 1.0, "b": 1.0}) + result.write_text( + '{"n_total_trials": 2, "stats": %s}' % stats, encoding="utf-8" + ) + proc = run(result, "--json") + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIs(strict_loads(proc.stdout)["ok"], True) + + def test_json_wrong_shaped_trial_fields_do_not_crash(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + (result.parent / "a__abc" / "result.json").write_text( + json.dumps({"task_name": 5, "trial_name": ["x"], "verifier_result": []}), + encoding="utf-8", + ) + write_trial_text( + result, + json.dumps({"task_name": "b", "verifier_result": {"rewards": [1.0]}}), + ) + proc = run(result, "--json") + self.assertEqual(proc.returncode, 1) + self.assertNotIn("Traceback", proc.stderr) + data = strict_loads(proc.stdout) + self.assertEqual([t["reward"] for t in data["trials"]], [None, None]) + self.assertEqual(len(data["problems"]), 2) + + def test_json_wrong_shaped_exception_info_is_an_error(self): + for index, info in enumerate(('"boom"', "[1]", '{"exception_type": 5}')): + with self.subTest(info=info): + result = make_job(self.subdir(f"exc-{index}"), {"a": 1.0, "b": 1.0}) + write_trial_text( + result, '{"task_name": "b", "exception_info": %s}' % info + ) + proc = run(result, "--json") + self.assertEqual(proc.returncode, 1, info) + data = strict_loads(proc.stdout) + self.assertEqual(data["trials"][1]["error"], "unknown") + self.assertTrue( + any("b errored: unknown" in p for p in data["problems"]) + ) + + def test_text_mode_failure_message_unchanged(self): + proc = run(self.tmp / "missing.json") + self.assertEqual(proc.returncode, 1) + self.assertEqual(proc.stdout, "") + self.assertIn("FAIL no such file", proc.stderr) + + def test_json_non_finite_reward_is_null_and_strict(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + # json.dumps writes NaN / Infinity, which Python's json module reads back. + write_trial_text( + result, + '{"task_name": "b", "verifier_result": {"rewards": {"reward": NaN}}}', + ) + (result.parent / "a__abc" / "result.json").write_text( + '{"task_name": "a", "verifier_result": {"rewards": {"reward": Infinity}}}', + encoding="utf-8", + ) + proc = run(result, "--json") + self.assertEqual(proc.returncode, 1) + data = strict_loads(proc.stdout) + self.assertEqual([t["reward"] for t in data["trials"]], [None, None]) + self.assertIs(data["ok"], False) + + def test_json_huge_int_reward_is_null_and_strict(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + # 400 digits parse as an int but cannot convert to float. + huge = "1" + "0" * 400 + for sign in ("", "-"): + with self.subTest(sign=sign): + write_trial_text( + result, + '{"task_name": "b", "verifier_result": {"rewards": {"reward": %s%s}}}' + % (sign, huge), + ) + proc = run(result, "--json") + self.assertEqual(proc.returncode, 1) + self.assertNotIn("Traceback", proc.stderr) + data = strict_loads(proc.stdout) + self.assertIsNone(data["trials"][1]["reward"]) + self.assertTrue( + any("b produced no reward" in p for p in data["problems"]) + ) + + def test_text_mode_huge_int_reward_has_no_traceback(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + write_trial_text( + result, + '{"task_name": "b", "verifier_result": {"rewards": {"reward": 1%s}}}' + % ("0" * 400), + ) + proc = run(result) + self.assertEqual(proc.returncode, 1) + self.assertNotIn("Traceback", proc.stderr) + self.assertIn("b produced no reward", proc.stdout) + + def test_json_huge_int_reward_with_allow_errors_still_fails(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + write_trial_text( + result, + '{"task_name": "b", "verifier_result": {"rewards": {"reward": 1%s}}}' + % ("0" * 400), + ) + proc = run(result, "--json", "--allow-errors") + self.assertEqual(proc.returncode, 1) + self.assertIsNone(strict_loads(proc.stdout)["trials"][1]["reward"]) + + def test_json_over_digit_limit_int_is_structured_failure(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + # Literal text: the decoder's integer digit limit is 4300 digits. + write_trial_text( + result, + '{"task_name": "b", "verifier_result": {"rewards": {"reward": 1%s}}}' + % ("0" * 5000), + ) + data = self.assert_structured_failure(run(result, "--json"), "b__abc") + self.assertEqual(data["trials"], []) + + def test_json_over_digit_limit_int_in_job_is_structured_failure(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + result.write_text('{"n_total_trials": 1%s}' % ("0" * 5000), encoding="utf-8") + self.assert_structured_failure(run(result, "--json"), "result.json") + + def test_json_deeply_nested_trial_is_structured_failure(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + write_trial_text(result, "[" * 100000 + "]" * 100000) + data = self.assert_structured_failure(run(result, "--json"), "b__abc") + self.assertEqual(data["trials"], []) + + def test_json_deeply_nested_job_is_structured_failure(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + result.write_text( + '{"stats": ' + "[" * 100000 + "]" * 100000 + "}", encoding="utf-8" + ) + # Python 3.14+ parses this depth instead of raising RecursionError, so the job + # then fails on the missing trial count rather than on a load error. Either is a + # structured failure; a traceback or a pass is the regression. + data = self.assert_structured_failure(run(result, "--json"), "") + self.assertTrue( + any("result.json" in p or "expected 2 trials" in p for p in data["problems"]), + data["problems"], + ) + + def test_text_mode_deeply_nested_job_fails_without_traceback(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + result.write_text("[" * 100000, encoding="utf-8") + proc = run(result) + self.assertEqual(proc.returncode, 1) + self.assertEqual(proc.stdout, "") + self.assertIn("FAIL", proc.stderr) + self.assertNotIn("Traceback", proc.stderr) + + def test_allow_errors_tolerates_errored_trial(self): + # --allow-errors is for exploratory runs: an errored trial is reported, not failed. + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + write_trial_text( + result, + json.dumps( + {"task_name": "b", "exception_info": {"exception_type": "Timeout"}} + ), + ) + proc = run(result, "--json", "--allow-errors") + self.assertEqual(proc.returncode, 0) + data = strict_loads(proc.stdout) + self.assertEqual(data["trials"][1]["error"], "Timeout") + + def test_default_output_unchanged(self): + proc = run(make_job(self.tmp, {"a": 1.0, "b": 1.0})) + self.assertEqual(proc.returncode, 0) + self.assertTrue(proc.stdout.rstrip().endswith("OK 2 trial(s) at or above 1.0")) + + +if __name__ == "__main__": + unittest.main() diff --git a/tools/tests/test_compare_harbor_report_only.py b/tools/tests/test_compare_harbor_report_only.py new file mode 100644 index 0000000..247c6fa --- /dev/null +++ b/tools/tests/test_compare_harbor_report_only.py @@ -0,0 +1,64 @@ +"""compare_harbor_skill.py --report-only reports on whichever arms have finished.""" + +from __future__ import annotations + +import json +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +TOOL = Path(__file__).resolve().parents[1] / "compare_harbor_skill.py" +SKILL = "dpnp-quickstart" + + +def make_arm(jobs: Path, arm: str, reward: float) -> None: + job = jobs / f"{SKILL}-{arm}" + trial = job / "task__abc" + trial.mkdir(parents=True) + (job / "result.json").write_text(json.dumps({"stats": {}}), encoding="utf-8") + body = {"task_name": "task", "verifier_result": {"rewards": {"reward": reward}}} + (trial / "result.json").write_text(json.dumps(body), encoding="utf-8") + + +def report_only(jobs: Path) -> subprocess.CompletedProcess: + return subprocess.run( + [ + sys.executable, + str(TOOL), + "--skill", + SKILL, + "--model", + "m", + "--report-only", + "--jobs-dir", + str(jobs), + ], + capture_output=True, + text=True, + ) + + +class ReportOnlyTest(unittest.TestCase): + def test_baseline_without_candidate_still_reports(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + jobs = Path(tmp) + make_arm(jobs, "no-skill", 0.5) + result = report_only(jobs) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("`no_skill`: 0.500", result.stdout) + self.assertNotIn("candidate − no_skill =", result.stdout) + + def test_both_arms_report_the_delta(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + jobs = Path(tmp) + make_arm(jobs, "no-skill", 0.5) + make_arm(jobs, "candidate", 1.0) + result = report_only(jobs) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("candidate − no_skill = +0.500", result.stdout) + + +if __name__ == "__main__": + unittest.main() diff --git a/tools/tests/test_run_evals_skill_filter.py b/tools/tests/test_run_evals_skill_filter.py new file mode 100644 index 0000000..268739b --- /dev/null +++ b/tools/tests/test_run_evals_skill_filter.py @@ -0,0 +1,41 @@ +"""`run_evals.py --skill NAME` limits validation and scoring to one skill.""" + +import json +import subprocess +import sys +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[2] +SCRIPT = REPO_ROOT / "tools" / "run_evals.py" + + +def run(*args): + return subprocess.run( + [sys.executable, str(SCRIPT), *args], capture_output=True, text=True, cwd=REPO_ROOT + ) + + +def first_skill_with_evals(): + path = sorted((REPO_ROOT / "skills").glob("*/evals/evals.json"))[0] + return path.parent.parent.name, json.loads(path.read_text(encoding="utf-8")) + + +def test_validate_one_skill_counts_only_its_cases(): + name, data = first_skill_with_evals() + result = run("--validate", "--skill", name) + assert result.returncode == 0, result.stderr + assert result.stdout.strip() == f"OK 1 eval file(s), {len(data['evals'])} case(s)" + + +def test_score_one_skill_ignores_other_skills_cases(tmp_path): + name, data = first_skill_with_evals() + answers = tmp_path / "answers.json" + answers.write_text("{}", encoding="utf-8") + result = run("--answers", str(answers), "--skill", name, "--fail-under", "0") + assert f"0/{len(data['evals'])} passed" in result.stdout + + +def test_unknown_skill_is_an_error(): + result = run("--validate", "--skill", "no-such-skill") + assert result.returncode != 0 + assert "no-such-skill" in result.stderr diff --git a/tools/validate_skills.py b/tools/validate_skills.py index 34aae54..8cd5296 100644 --- a/tools/validate_skills.py +++ b/tools/validate_skills.py @@ -209,6 +209,23 @@ ) COMMIT_RE = re.compile(r"^[0-9a-f]{40}$") +# The keys a skills.yaml entry may carry, and the values intel-hw-class may take, as +# the header of skills.yaml documents them. parse_catalog keeps any key it is given, so +# without this a misspelt `intel-hw-clas` or `intel-product` is read, ignored, and +# reported as nothing: the field the author meant to set is simply absent. +CATALOG_KEYS = { + "name", + "maintainer", + "status", + "license", + "intel-products", + "intel-hw-class", + "intel-hw-validated-on", + "intel-source-ledger", + *EXTERNAL_KEYS, +} +HW_CLASSES = ("cpu", "gpu", "npu", "accelerator") + NAME_RE = re.compile(r"^[a-z0-9]+(?:-[a-z0-9]+)*$") DESCRIPTION_MAX = 1024 LINES_WARN = 250 @@ -851,6 +868,19 @@ def check_catalog( ) if not entry.get("maintainer"): report.error(f"skills.yaml: {name}: maintainer is required") + for key in sorted(set(entry) - CATALOG_KEYS): + report.error( + f"skills.yaml: {name}: unknown key {key!r}, which nothing reads — " + f"known keys are {sorted(CATALOG_KEYS)}" + ) + # An empty value means "does not apply", per the header, so only a value that + # is present has to be one of the classes. + hw_class = entry.get("intel-hw-class") or "" + if hw_class and hw_class not in HW_CLASSES: + report.error( + f"skills.yaml: {name}: intel-hw-class must be one of {list(HW_CLASSES)}, " + f"got {hw_class!r}" + ) has_perf = has_files(skill_dir / "perf") @@ -1206,6 +1236,17 @@ def check_bundles(catalog: dict[str, dict[str, str]], report: Report) -> None: f"{where}: unknown key {key!r} — the skills.sh schema allows only " f"{sorted(BUNDLES_TOP_KEYS)}" ) + # skills.sh types both autocomplete fields as string. A number or a list is + # not a URL, and leaving them unchecked made a bad file read as OK. + # format: uri is not asserted here. + for field in ("$schema", "schema"): + if field not in data: + continue + value = data[field] + if not isinstance(value, str): + report.error( + f"{where}: {field} must be a string, got {type(value).__name__}" + ) not_grouped = data.get("notGrouped", "bottom") if not_grouped not in NOT_GROUPED: report.error( @@ -1610,11 +1651,59 @@ def doc_links(docs: bool, check_links: bool, report: Report) -> list[tuple[str, return links +def self_test_bundle_schema_types() -> int: + """A non-string skills.sh schema field must fail; a string must not. + + The published skills.sh schema types ``$schema`` and ``schema`` as string. + This does not assert ``format: uri``. + """ + import tempfile + + global BUNDLES_PATH + saved = BUNDLES_PATH + failures: list[str] = [] + try: + live = json.loads(saved.read_text(encoding="utf-8")) + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + good = root / "good.json" + good.write_text(json.dumps(live), encoding="utf-8") + BUNDLES_PATH = good + report = Report() + check_bundles({}, report) + if any("must be a string" in error for error in report.errors): + failures.append("a string $schema was rejected") + + bad_data = dict(live) + bad_data["$schema"] = 1 + bad_data["schema"] = ["not-a-uri"] + bad = root / "bad.json" + bad.write_text(json.dumps(bad_data), encoding="utf-8") + BUNDLES_PATH = bad + report = Report() + check_bundles({}, report) + for field in ("$schema", "schema"): + if not any(f"{field} must be a string" in error for error in report.errors): + failures.append(f"non-string {field} was accepted") + finally: + BUNDLES_PATH = saved + if failures: + print("FAIL --self-test: " + "; ".join(failures), file=sys.stderr) + return 1 + print("PASS --self-test: non-string skills.sh schema fields are rejected") + return 0 + + def main() -> int: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument( "--strict", action="store_true", help="treat warnings as failures" ) + parser.add_argument( + "--self-test", + action="store_true", + help="assert that a non-string skills.sh $schema or schema field fails", + ) parser.add_argument( "--check-links", action="store_true", @@ -1630,6 +1719,8 @@ def main() -> int: "and evaluation/. Run weekly by link-rot.yml rather than on every PR.", ) args = parser.parse_args() + if args.self_test: + return self_test_bundle_schema_types() report = Report() skill_dirs = sorted(p for p in SKILLS_DIR.iterdir() if p.is_dir()) From 6f64deccac34d100722c3ba51249d5e9ba7ad3a1 Mon Sep 17 00:00:00 2001 From: Alexander Rybkin Date: Mon, 26 Oct 2026 15:24:21 -0700 Subject: [PATCH 2/5] test: make run_evals --skill tests collectable by unittest The file used bare pytest-style functions, so the unittest-based Tool tests step collected none of them and a broken --skill filter stayed green. The unknown-skill test now checks the tool's own FAIL line, since argparse rejecting the flag on main satisfied the old assertion. Signed-off-by: Alexander Rybkin --- tools/tests/test_run_evals_skill_filter.py | 38 +++++++++++++--------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/tools/tests/test_run_evals_skill_filter.py b/tools/tests/test_run_evals_skill_filter.py index 268739b..79eb676 100644 --- a/tools/tests/test_run_evals_skill_filter.py +++ b/tools/tests/test_run_evals_skill_filter.py @@ -3,6 +3,8 @@ import json import subprocess import sys +import tempfile +import unittest from pathlib import Path REPO_ROOT = Path(__file__).resolve().parents[2] @@ -20,22 +22,28 @@ def first_skill_with_evals(): return path.parent.parent.name, json.loads(path.read_text(encoding="utf-8")) -def test_validate_one_skill_counts_only_its_cases(): - name, data = first_skill_with_evals() - result = run("--validate", "--skill", name) - assert result.returncode == 0, result.stderr - assert result.stdout.strip() == f"OK 1 eval file(s), {len(data['evals'])} case(s)" +class SkillFilterTest(unittest.TestCase): + def test_validate_one_skill_counts_only_its_cases(self): + name, data = first_skill_with_evals() + n = len(data["evals"]) + result = run("--validate", "--skill", name) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout.strip(), f"OK 1 eval file(s), {n} case(s)") + def test_score_one_skill_ignores_other_skills_cases(self): + name, data = first_skill_with_evals() + n = len(data["evals"]) + with tempfile.TemporaryDirectory() as tmp: + answers = Path(tmp) / "answers.json" + answers.write_text("{}", encoding="utf-8") + result = run("--answers", str(answers), "--skill", name, "--fail-under", "0") + self.assertIn(f"0/{n} passed", result.stdout) -def test_score_one_skill_ignores_other_skills_cases(tmp_path): - name, data = first_skill_with_evals() - answers = tmp_path / "answers.json" - answers.write_text("{}", encoding="utf-8") - result = run("--answers", str(answers), "--skill", name, "--fail-under", "0") - assert f"0/{len(data['evals'])} passed" in result.stdout + def test_unknown_skill_is_an_error(self): + result = run("--validate", "--skill", "no-such-skill") + self.assertNotEqual(result.returncode, 0) + self.assertIn("FAIL no skills/no-such-skill/evals/evals.json found", result.stderr) -def test_unknown_skill_is_an_error(): - result = run("--validate", "--skill", "no-such-skill") - assert result.returncode != 0 - assert "no-such-skill" in result.stderr +if __name__ == "__main__": + unittest.main() From e741fc246021f8ace2f34c5fa0a95c98b69a5538 Mon Sep 17 00:00:00 2001 From: Alexander Rybkin Date: Mon, 26 Oct 2026 15:24:37 -0700 Subject: [PATCH 3/5] fix: report an unreadable trial result as one FAIL line in compare_harbor_skill check_harbor_job.load() now raises LoadError instead of exiting, and summarize() did not catch it, so a malformed trial result.json ended --report-only and the post-run summary in a traceback. Signed-off-by: Alexander Rybkin --- tools/compare_harbor_skill.py | 8 ++++++-- tools/tests/test_compare_harbor_report_only.py | 12 ++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/tools/compare_harbor_skill.py b/tools/compare_harbor_skill.py index 0a36a5b..1fb45c6 100644 --- a/tools/compare_harbor_skill.py +++ b/tools/compare_harbor_skill.py @@ -55,7 +55,7 @@ from datetime import datetime from pathlib import Path -from check_harbor_job import collect_trials, reward_of +from check_harbor_job import LoadError, collect_trials, reward_of REPO_ROOT = Path(__file__).resolve().parent.parent SUITES_PATH = REPO_ROOT / "evaluation" / "harbor" / "suites.json" @@ -312,7 +312,11 @@ def summarize(job_dir: Path) -> dict: effort: dict = {} per_task_effort: dict[str, dict] = {} scored = 0 - for trial in collect_trials(result_path): + try: + trials = collect_trials(result_path) + except LoadError as exc: + die(str(exc)) + for trial in trials: name = trial.get("task_name") or "" if trial.get("exception_info"): kind = (trial["exception_info"] or {}).get("exception_type") or "unknown" diff --git a/tools/tests/test_compare_harbor_report_only.py b/tools/tests/test_compare_harbor_report_only.py index 247c6fa..58ef999 100644 --- a/tools/tests/test_compare_harbor_report_only.py +++ b/tools/tests/test_compare_harbor_report_only.py @@ -59,6 +59,18 @@ def test_both_arms_report_the_delta(self) -> None: self.assertEqual(result.returncode, 0, result.stderr) self.assertIn("candidate − no_skill = +0.500", result.stdout) + def test_malformed_trial_result_is_one_fail_line(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + jobs = Path(tmp) + job = jobs / f"{SKILL}-no-skill" + (job / "task__a").mkdir(parents=True) + (job / "result.json").write_text(json.dumps({"stats": {}}), encoding="utf-8") + (job / "task__a" / "result.json").write_text("{bad", encoding="utf-8") + result = report_only(jobs) + self.assertEqual(result.returncode, 1) + self.assertTrue(result.stderr.startswith("FAIL"), result.stderr) + self.assertNotIn("Traceback", result.stderr) + if __name__ == "__main__": unittest.main() From 301b5055f47b967debad286f264e61e855c0f949 Mon Sep 17 00:00:00 2001 From: Alexander Rybkin Date: Mon, 26 Oct 2026 15:25:00 -0700 Subject: [PATCH 4/5] fix: name a non-finite reward instead of asking for reward.txt reward_of() returns None for NaN, infinity and overflowing rewards, so the text report told the author to write a reward file that was already written. A present but non-finite or overflowing reward now gets its own message; rc and the --json rows are unchanged. Signed-off-by: Alexander Rybkin --- tools/check_harbor_job.py | 7 +++++++ tools/tests/test_check_harbor_job_json.py | 17 +++++++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/tools/check_harbor_job.py b/tools/check_harbor_job.py index ed6a88d..0deadda 100644 --- a/tools/check_harbor_job.py +++ b/tools/check_harbor_job.py @@ -184,6 +184,13 @@ def main() -> int: problems.append(f"{name} errored: {kind}") continue + raw = rewards_of(trial).get(REWARD_KEY) + if reward is None and isinstance(raw, (int, float)): + # Present but NaN, infinite, or too large for a float. + say(f"ERROR {name}: reward is not a finite number ({raw!r})") + problems.append(f"{name} reported a non-finite reward ({raw!r})") + continue + if reward is None: say(f"ERROR {name}: verifier reported no '{REWARD_KEY}'") problems.append( diff --git a/tools/tests/test_check_harbor_job_json.py b/tools/tests/test_check_harbor_job_json.py index c96d30f..e15a51d 100644 --- a/tools/tests/test_check_harbor_job_json.py +++ b/tools/tests/test_check_harbor_job_json.py @@ -216,6 +216,19 @@ def test_json_non_finite_reward_is_null_and_strict(self): self.assertEqual([t["reward"] for t in data["trials"]], [None, None]) self.assertIs(data["ok"], False) + def test_text_mode_nan_reward_is_named_non_finite(self): + result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) + write_trial_text( + result, + '{"task_name": "b", "verifier_result": {"rewards": {"reward": NaN}}}', + ) + for extra in ((), ("--allow-errors",)): + with self.subTest(extra=extra): + proc = run(result, *extra) + self.assertEqual(proc.returncode, 1) + self.assertIn("non-finite", proc.stdout) + self.assertNotIn("reward.txt", proc.stdout + proc.stderr) + def test_json_huge_int_reward_is_null_and_strict(self): result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) # 400 digits parse as an int but cannot convert to float. @@ -233,7 +246,7 @@ def test_json_huge_int_reward_is_null_and_strict(self): data = strict_loads(proc.stdout) self.assertIsNone(data["trials"][1]["reward"]) self.assertTrue( - any("b produced no reward" in p for p in data["problems"]) + any("b reported a non-finite reward" in p for p in data["problems"]) ) def test_text_mode_huge_int_reward_has_no_traceback(self): @@ -246,7 +259,7 @@ def test_text_mode_huge_int_reward_has_no_traceback(self): proc = run(result) self.assertEqual(proc.returncode, 1) self.assertNotIn("Traceback", proc.stderr) - self.assertIn("b produced no reward", proc.stdout) + self.assertIn("b reported a non-finite reward", proc.stdout) def test_json_huge_int_reward_with_allow_errors_still_fails(self): result = make_job(self.tmp, {"a": 1.0, "b": 1.0}) From 3b7df36e6d3e9e5ad95a176ee29a8216c28c54a2 Mon Sep 17 00:00:00 2001 From: Alexander Rybkin Date: Mon, 26 Oct 2026 15:25:33 -0700 Subject: [PATCH 5/5] docs: list the tool unit tests and validator self-test in the CI table The validate.yml row names each blocking step, but left out the two blocking steps this branch adds: Tool tests and the validate_skills.py self-test. Signed-off-by: Alexander Rybkin --- MAINTAINERS.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/MAINTAINERS.md b/MAINTAINERS.md index 8b33955..859cead 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -284,7 +284,7 @@ request the annotation is for. | Workflow | Job | Runs on | Blocks? | |---|---|---|---| -| `validate.yml` | `validate` — `validate_skills.py`, `run_evals.py --validate`, task leakage and its self-test, the overlap self-test, the staleness detector's self-test, link check | every PR | yes | +| `validate.yml` | `validate` — `validate_skills.py` and its self-test, `run_evals.py --validate`, the tool unit tests under `tools/tests`, task leakage and its self-test, the overlap self-test, the staleness detector's self-test, link check | every PR | yes | | `validate.yml` | `validate` — skill overlap, `--advisory` | every PR | only on containment 1.0 | | `validate.yml` | `install` — the installer resolves, lists, and installs from the catalog | every PR | yes | | `harbor-smoke.yml` | the oracle arm over every task in `tasks/` | PRs touching tasks or skills | yes | @@ -339,7 +339,7 @@ Everything else runs offline. | Script | What it does | |---|---| -| `validate_skills.py` | Level 1: every structural check, the catalog ↔ tree bijection, and `skills.sh.json` against the catalog | +| `validate_skills.py` | Level 1: every structural check, the catalog ↔ tree bijection, and `skills.sh.json` against the catalog; `--self-test` asserts that a non-string skills.sh schema field is rejected | | `run_evals.py` | validates eval files against their schema; scores recorded answers | | `compare_harbor_skill.py` | runs and reports the three-arm differential, with cost and time | | `check_harbor_job.py` | asserts a harbor run's trial count and reward floor |