mirror of
https://github.com/magnus919/agent-skills.git
synced 2026-09-20 08:06:35 +03:00
fix: enforce eval coverage ratchet for complete changed skill directories (#107)
Wire the Phase 3 ratchet into CI by passing the PR base SHA to eval-coverage.py --modified-from. Expand changed-skill detection from SKILL.md-only diffs to the entire skill directory so that references, scripts, fixtures, README, and eval manifest edits all count as modifications. Add a monotonic coverage floor that fails CI when coverage decreases between the base and candidate revisions. Add script tests for ratchet-mode detection and coverage-decrease behaviour. Update AGENTS.md and CONTRIBUTING.md to describe the behaviour CI now enforces. Closes #102 Signed-off-by: Magnus Hedemark <magnus919@pm.me>
This commit is contained in:
Magnus Hedemark
@@ -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
|
||||
|
||||
@@ -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 <base-sha>` 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
|
||||
|
||||
|
||||
+1
-1
@@ -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 <base-sha>`. 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
|
||||
|
||||
|
||||
@@ -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 <root>/<skill-name>/SKILL.md or
|
||||
# <root>/bundles/<bundle-name>/skills/<skill-name>/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(
|
||||
|
||||
@@ -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()
|
||||
Reference in New Issue
Block a user