diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 968d579..b7c06d4 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -36,6 +36,10 @@ jobs: run: ruby scripts/gen-codex-plugin.rb - name: Report eval coverage run: python3 scripts/eval-coverage.py + - name: Enforce eval coverage ratchet + env: + EVAL_RATCHET_BASE: ${{ github.event.pull_request.base.sha || github.event.before }} + run: python3 scripts/eval-coverage.py --modified-from "$EVAL_RATCHET_BASE" - name: Test llms.txt generator run: ruby scripts/test-gen-llms-txt.rb - name: Validate llms.txt catalog diff --git a/AGENTS.md b/AGENTS.md index 9d9acd0..93ae4c9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -99,7 +99,7 @@ Every skill description must start with an imperative verb and define both when Every new skill must include `evals/evals.json` with at least five representative output-quality cases. Each case needs a realistic prompt, an expected outcome, and observable assertions. Trigger-only checks (should-trigger / should-not-trigger probes) are harness-specific and belong in a separate test set, not in `evals/evals.json`. -Existing skills are grandfathered via `scripts/grandfathered-skills.txt`. As overall eval coverage climbs past 25%, modified skills without evals receive a warning; past 50%, they fail CI. The coverage report is available via `python3 scripts/eval-coverage.py`. +Existing skills are grandfathered via `scripts/grandfathered-skills.txt`. As overall eval coverage climbs past 25%, modified skills without evals receive a warning; past 50%, they fail CI. The coverage report is available via `python3 scripts/eval-coverage.py`. The ratchet is enforced in CI via `python3 scripts/eval-coverage.py --modified-from ` on every pull request. A skill is considered modified when any tracked file under its directory changes, not only `SKILL.md`. Coverage must not decrease between the base revision and the candidate; a decrease fails CI. ## Best Practices diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c8d9b87..2156fdf 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -20,7 +20,7 @@ Each skill must: - Use relative links that work from a fresh clone. - Use an imperative-verb description that defines both positive and negative trigger boundaries. - Describe when the skill should be loaded, and identify the nearest alternative when overlap matters. -- Include `evals/evals.json` with at least five representative output-quality cases for every new skill. Existing skills are grandfathered via `scripts/grandfathered-skills.txt`; the ratchet tightens as overall coverage climbs. +- Include `evals/evals.json` with at least five representative output-quality cases for every new skill. Existing skills are grandfathered via `scripts/grandfathered-skills.txt`; the ratchet tightens as overall coverage climbs. The ratchet runs in CI on every pull request via `python3 scripts/eval-coverage.py --modified-from `. A skill counts as modified when any tracked file under its directory changes. Coverage must not decrease between the base revision and the candidate. ## Development diff --git a/scripts/eval-coverage.py b/scripts/eval-coverage.py index f3a4c93..d0614bf 100644 --- a/scripts/eval-coverage.py +++ b/scripts/eval-coverage.py @@ -23,11 +23,19 @@ GRANDFATHER_FILE = ROOT / "scripts" / "grandfathered-skills.txt" WARN_THRESHOLD = 25 # modified skills without evals get a warning FAIL_THRESHOLD = 50 # modified skills without evals fail CI +# Pathspec that matches every tracked file under a canonical skill directory. +# A canonical skill lives at //SKILL.md or +# /bundles//skills//SKILL.md. The glob +# ``*/SKILL.md`` covers the first shape; ``bundles/*/skills/*/SKILL.md`` +# covers the second. We use the same glob for both ls-files and diff so +# that modified-skill detection sees the same universe as find_skills(). +SKILL_PATHSPEC = ":(glob)**/SKILL.md" + def find_skills() -> list[Path]: """Find all canonical skill directories via git-tracked SKILL.md files.""" result = subprocess.run( - ["git", "ls-files", "-z", "--", "*/SKILL.md"], + ["git", "ls-files", "-z", "--", SKILL_PATHSPEC], cwd=ROOT, check=True, capture_output=True, @@ -81,20 +89,86 @@ def count_references(skill_name: str, all_skill_dirs: list[Path]) -> int: def modified_skills(base_ref: str) -> set[Path]: - """Return skill directories with changes between base_ref and HEAD.""" + """Return skill directories with any tracked file changed since base_ref. + + A skill is considered modified when *any* file under its directory + changes — not just SKILL.md. This covers references, scripts, + fixtures, README, and eval manifests. + """ result = subprocess.run( - ["git", "diff", "--name-only", base_ref, "HEAD", "--", "*/SKILL.md"], + ["git", "diff", "--name-only", base_ref, "HEAD"], cwd=ROOT, capture_output=True, text=True, ) - modified = set() - for line in result.stdout.strip().splitlines(): - if line and "/agent-council/profiles/skills/" not in line: - modified.add(Path(line).parent) + changed_files = [ + line for line in result.stdout.strip().splitlines() + if line and "/agent-council/profiles/skills/" not in line + ] + # Map each changed file to its owning skill directory by checking + # whether the file path starts with a known skill directory prefix. + known_skills = find_skills() + modified: set[Path] = set() + for changed in changed_files: + changed_path = Path(changed) + for skill_dir in known_skills: + try: + changed_path.relative_to(skill_dir) + modified.add(skill_dir) + break + except ValueError: + continue return modified +def coverage_decreased(base_ref: str) -> tuple[bool, float, float]: + """Compare eval coverage between base_ref and HEAD. + + Returns (decreased, base_pct, head_pct). Coverage is the percentage + of canonical skills that have a non-empty evals/evals.json. + """ + head_skills = find_skills() + head_with = sum(1 for s in head_skills if check_evals(s)[0]) + head_pct = (head_with / len(head_skills) * 100) if head_skills else 0.0 + + # Count skills with evals at the base revision. + result = subprocess.run( + ["git", "ls-tree", "-r", "--name-only", base_ref], + cwd=ROOT, + capture_output=True, + text=True, + ) + base_skill_dirs: set[Path] = set() + for line in result.stdout.strip().splitlines(): + if ( + line + and line.endswith("/SKILL.md") + and "/agent-council/profiles/skills/" not in line + ): + base_skill_dirs.add(Path(line).parent) + + base_with = 0 + for skill_dir in base_skill_dirs: + evals_path = f"{skill_dir}/evals/evals.json" + cat = subprocess.run( + ["git", "show", f"{base_ref}:{evals_path}"], + cwd=ROOT, + capture_output=True, + text=True, + ) + if cat.returncode != 0: + continue + try: + data = json.loads(cat.stdout) + if isinstance(data, dict) and isinstance(data.get("evals"), list) and len(data["evals"]) > 0: + base_with += 1 + except (json.JSONDecodeError, ValueError): + pass + + base_pct = (base_with / len(base_skill_dirs) * 100) if base_skill_dirs else 0.0 + return head_pct < base_pct, base_pct, head_pct + + def main() -> int: parser = argparse.ArgumentParser(description="Eval coverage report and ratchet") parser.add_argument("--json", action="store_true", help="JSON output") @@ -148,6 +222,14 @@ def main() -> int: f"(coverage {coverage_pct:.1f}% >= {WARN_THRESHOLD}% — evals recommended)" ) + # Monotonic coverage floor: fail if coverage decreased. + decreased, base_pct, head_pct = coverage_decreased(args.modified_from) + if decreased: + ratchet_errors.append( + f"eval coverage decreased from {base_pct:.1f}% to {head_pct:.1f}% " + f"(base {args.modified_from} → HEAD) — coverage must not regress" + ) + if args.json: print( json.dumps( diff --git a/scripts/test-eval-coverage.py b/scripts/test-eval-coverage.py new file mode 100644 index 0000000..afbc0d7 --- /dev/null +++ b/scripts/test-eval-coverage.py @@ -0,0 +1,203 @@ +#!/usr/bin/env python3 +"""Tests for scripts/eval-coverage.py ratchet logic. + +Uses a temporary git repository to verify modified-skill detection, +coverage-decrease detection, and threshold behavior. +""" + +import json +import os +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +# Import the module under test. The script is named eval-coverage.py +# (hyphenated), so we load it via importlib rather than a normal import. +import importlib.util # noqa: E402 + +SCRIPT_DIR = Path(__file__).resolve().parent +_spec = importlib.util.spec_from_file_location( + "eval_coverage", SCRIPT_DIR / "eval-coverage.py" +) +eval_coverage = importlib.util.module_from_spec(_spec) +sys.modules["eval_coverage"] = eval_coverage +_spec.loader.exec_module(eval_coverage) # type: ignore[union-attr] + + +def git(repo: str, *args: str) -> str: + """Run a git command in *repo* and return stdout.""" + result = subprocess.run( + ["git", *args], + cwd=repo, + capture_output=True, + text=True, + check=True, + ) + return result.stdout.strip() + + +def write_skill(repo: str, name: str, evals: list[dict] | None = None) -> None: + """Create a minimal skill directory with an optional evals manifest.""" + skill_dir = Path(repo) / name + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: Test skill {name}.\n---\n\n# {name}\n" + ) + if evals is not None: + evals_dir = skill_dir / "evals" + evals_dir.mkdir(exist_ok=True) + (evals_dir / "evals.json").write_text( + json.dumps({"skill_name": name, "evals": evals}, indent=2) + ) + + +def make_case(case_id: str) -> dict: + return {"id": case_id, "prompt": "test", "expected_output": "ok", "assertions": ["a"]} + + +class TestModifiedSkills(unittest.TestCase): + """modified_skills() must detect changes to any file under a skill dir.""" + + def setUp(self) -> None: + self.tmp = tempfile.TemporaryDirectory() + self.repo = self.tmp.name + git(self.repo, "init", "-b", "main") + git(self.repo, "config", "user.email", "test@test.invalid") + git(self.repo, "config", "user.name", "Test") + + def tearDown(self) -> None: + self.tmp.cleanup() + + def _commit(self, msg: str) -> str: + git(self.repo, "add", "-A") + git(self.repo, "commit", "-m", msg) + return git(self.repo, "rev-parse", "HEAD") + + def test_skill_md_change_detected(self) -> None: + write_skill(self.repo, "alpha") + base = self._commit("initial") + (Path(self.repo) / "alpha" / "SKILL.md").write_text("changed") + self._commit("edit SKILL.md") + old_root = eval_coverage.ROOT + eval_coverage.ROOT = Path(self.repo) + try: + mods = eval_coverage.modified_skills(base) + finally: + eval_coverage.ROOT = old_root + self.assertIn(Path("alpha"), mods) + + def test_reference_change_detected(self) -> None: + write_skill(self.repo, "beta") + ref_dir = Path(self.repo) / "beta" / "references" + ref_dir.mkdir() + (ref_dir / "guide.md").write_text("v1") + base = self._commit("initial") + (ref_dir / "guide.md").write_text("v2") + self._commit("edit reference") + old_root = eval_coverage.ROOT + eval_coverage.ROOT = Path(self.repo) + try: + mods = eval_coverage.modified_skills(base) + finally: + eval_coverage.ROOT = old_root + self.assertIn(Path("beta"), mods) + + def test_eval_manifest_deletion_detected(self) -> None: + write_skill(self.repo, "gamma", evals=[make_case("c1")]) + base = self._commit("initial") + (Path(self.repo) / "gamma" / "evals" / "evals.json").unlink() + self._commit("delete evals") + old_root = eval_coverage.ROOT + eval_coverage.ROOT = Path(self.repo) + try: + mods = eval_coverage.modified_skills(base) + finally: + eval_coverage.ROOT = old_root + self.assertIn(Path("gamma"), mods) + + def test_unrelated_file_not_detected(self) -> None: + write_skill(self.repo, "delta") + (Path(self.repo) / "README.md").write_text("root readme") + base = self._commit("initial") + (Path(self.repo) / "README.md").write_text("changed readme") + self._commit("edit root readme") + old_root = eval_coverage.ROOT + eval_coverage.ROOT = Path(self.repo) + try: + mods = eval_coverage.modified_skills(base) + finally: + eval_coverage.ROOT = old_root + self.assertNotIn(Path("delta"), mods) + + +class TestCoverageDecreased(unittest.TestCase): + """coverage_decreased() must detect a drop in eval coverage.""" + + def setUp(self) -> None: + self.tmp = tempfile.TemporaryDirectory() + self.repo = self.tmp.name + git(self.repo, "init", "-b", "main") + git(self.repo, "config", "user.email", "test@test.invalid") + git(self.repo, "config", "user.name", "Test") + + def tearDown(self) -> None: + self.tmp.cleanup() + + def _commit(self, msg: str) -> str: + git(self.repo, "add", "-A") + git(self.repo, "commit", "-m", msg) + return git(self.repo, "rev-parse", "HEAD") + + def test_decrease_detected(self) -> None: + write_skill(self.repo, "s1", evals=[make_case("c1")]) + write_skill(self.repo, "s2", evals=[make_case("c2")]) + base = self._commit("two skills with evals") + # Remove evals from s2. + (Path(self.repo) / "s2" / "evals" / "evals.json").unlink() + self._commit("remove s2 evals") + old_root = eval_coverage.ROOT + eval_coverage.ROOT = Path(self.repo) + try: + decreased, base_pct, head_pct = eval_coverage.coverage_decreased(base) + finally: + eval_coverage.ROOT = old_root + self.assertTrue(decreased) + self.assertAlmostEqual(base_pct, 100.0) + self.assertAlmostEqual(head_pct, 50.0) + + def test_no_decrease_when_stable(self) -> None: + write_skill(self.repo, "s1", evals=[make_case("c1")]) + write_skill(self.repo, "s2") + base = self._commit("one with evals, one without") + (Path(self.repo) / "s2" / "SKILL.md").write_text("edited") + self._commit("edit s2") + old_root = eval_coverage.ROOT + eval_coverage.ROOT = Path(self.repo) + try: + decreased, base_pct, head_pct = eval_coverage.coverage_decreased(base) + finally: + eval_coverage.ROOT = old_root + self.assertFalse(decreased) + self.assertAlmostEqual(base_pct, 50.0) + self.assertAlmostEqual(head_pct, 50.0) + + def test_increase_not_flagged(self) -> None: + write_skill(self.repo, "s1") + base = self._commit("no evals") + write_skill(self.repo, "s1", evals=[make_case("c1")]) + self._commit("add evals") + old_root = eval_coverage.ROOT + eval_coverage.ROOT = Path(self.repo) + try: + decreased, base_pct, head_pct = eval_coverage.coverage_decreased(base) + finally: + eval_coverage.ROOT = old_root + self.assertFalse(decreased) + self.assertAlmostEqual(base_pct, 0.0) + self.assertAlmostEqual(head_pct, 100.0) + + +if __name__ == "__main__": + unittest.main()