tools: stricter catalog validation, run_evals --skill, machine-readable Harbor job check

.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

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 |

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,61 +158,74 @@ 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 "<unnamed>"
+        name = label(trial.get("task_name")) or label(trial.get("trial_name")) or "<unnamed>"
         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
 
+        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:
-            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."
             )
             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}")
 
         # A rubric verifier emits components alongside the combined reward. Name
         # 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
 
 

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 "<unnamed>"
         if trial.get("exception_info"):
             kind = (trial["exception_info"] or {}).get("exception_type") or "unknown"
@@ -462,7 +466,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:

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/<name>/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

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()