From 476d7e11b02f2bee65a2368adff02d9366376791 Mon Sep 17 00:00:00 2001 From: username Date: Wed, 29 Jul 2026 17:42:58 -0400 Subject: [PATCH] feat(ci): add linting, formatting, coverage, and security configs Add ruff linter/formatter with pre-commit hooks, pytest-cov with 60% coverage threshold, CODEOWNERS, Dependabot for pip/GHA updates, and .env.example. Auto-fix existing ruff violations across eval_runner/ and scripts/. 10 agent-readiness criteria resolved: lint_config, formatter, pre_commit_hooks, naming_consistency, dead_code_detection, test_coverage_thresholds, test_performance_tracking, codeowners, dependency_update_automation, env_template. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- .env.example | 9 +++ .github/CODEOWNERS | 2 + .github/dependabot.yml | 18 ++++++ .github/workflows/validate.yml | 6 ++ .gitignore | 4 ++ .pre-commit-config.yaml | 7 +++ eval_runner/cli_adapter.py | 12 +--- eval_runner/fake_adapter.py | 4 +- eval_runner/grader.py | 36 +++++++++--- eval_runner/manifest.py | 4 +- eval_runner/openai_adapter.py | 40 +++++++------- eval_runner/paired.py | 13 +++-- eval_runner/path_safety.py | 6 +- eval_runner/release.py | 39 +++++-------- eval_runner/sandbox.py | 4 +- eval_runner/tests/test_paired.py | 26 +++++---- eval_runner/tests/test_release.py | 91 +++++++++++++++++++++++++------ eval_runner/tests/test_runner.py | 28 +++++----- pyproject.toml | 72 ++++++++++++++++++++++++ requirements-dev.txt | 3 + scripts/check-artifacts.py | 5 +- scripts/eval-coverage.py | 37 ++++++------- scripts/eval_validation.py | 8 +-- scripts/test-eval-coverage.py | 50 +++++++++-------- scripts/test-eval-validation.py | 27 ++++++--- 25 files changed, 366 insertions(+), 185 deletions(-) create mode 100644 .env.example create mode 100644 .github/CODEOWNERS create mode 100644 .github/dependabot.yml create mode 100644 .pre-commit-config.yaml create mode 100644 pyproject.toml diff --git a/.env.example b/.env.example new file mode 100644 index 0000000..2cacb2f --- /dev/null +++ b/.env.example @@ -0,0 +1,9 @@ +# Environment variables for the agent-skills repository +# Copy this file to .env and fill in the values + +# GitHub token for authenticated API access (optional, for git operations) +# GITHUB_TOKEN= + +# Evaluation model configuration (used by skill-eval CI) +# EVAL_BASE_URL= +# EVAL_MODEL= diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 0000000..c25df0e --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1,2 @@ +# Global owners +* @magnus919 diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..a974b60 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,18 @@ +version: 2 +updates: + - package-ecosystem: "pip" + directory: "/" + schedule: + interval: "weekly" + open-pull-requests-limit: 5 + versioning-strategy: increase + labels: + - "dependencies" + + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "monthly" + open-pull-requests-limit: 3 + labels: + - "dependencies" diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 88688ff..001bbbc 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -26,6 +26,10 @@ jobs: python-version: '3.12' - name: Install Python validator dependencies run: python3 -m pip install -r requirements-dev.txt + - name: Lint Python (ruff) + run: python3 -m ruff check scripts/ eval_runner/ + - name: Check Python formatting (ruff) + run: python3 -m ruff format --check scripts/ eval_runner/ - name: Validate skill format and links run: ruby scripts/validate-skills.rb - name: Test eval manifest validation @@ -34,6 +38,8 @@ jobs: run: python3 scripts/validate-evals.py - name: Test life-coach capability validation run: python3 -m unittest discover -s life-coach/tests -p 'test_*.py' + - name: Run core test suite with coverage + run: python3 -m pytest scripts/ eval_runner/tests/ -v --durations=10 --cov=scripts --cov=eval_runner --cov-fail-under=60 --cov-report=term-missing - name: Test changed-skill quality validation run: ruby scripts/test-validate-skill-quality.rb - name: Validate changed skill quality diff --git a/.gitignore b/.gitignore index b6a773d..48253f4 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,7 @@ # ─── Secrets & Configuration ────────────────────────────────── .env .env.* +!.env.example *.env .envrc *.pem @@ -42,6 +43,9 @@ venv/ virtualenv/ .uv/ .python-version +.coverage +coverage.xml +htmlcov/ # ─── macOS ──────────────────────────────────────────────────── .DS_Store diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml new file mode 100644 index 0000000..07adb89 --- /dev/null +++ b/.pre-commit-config.yaml @@ -0,0 +1,7 @@ +repos: + - repo: https://github.com/astral-sh/ruff-pre-commit + rev: v0.9.0 + hooks: + - id: ruff + args: [--fix] + - id: ruff-format diff --git a/eval_runner/cli_adapter.py b/eval_runner/cli_adapter.py index 211cc0e..cc7fe85 100644 --- a/eval_runner/cli_adapter.py +++ b/eval_runner/cli_adapter.py @@ -22,7 +22,6 @@ from __future__ import annotations import os import subprocess import time -from pathlib import Path from .models import AdapterInput, AdapterOutput, ExitStatus, ToolEvent @@ -82,16 +81,9 @@ class CliSubprocessAdapter: ) elapsed_ms = (time.monotonic() - start) * 1000 - if proc.returncode == 0: - exit_status = ExitStatus.COMPLETED - else: - exit_status = ExitStatus.ERROR + exit_status = ExitStatus.COMPLETED if proc.returncode == 0 else ExitStatus.ERROR - artifacts = [ - f.name - for f in input.output_dir.iterdir() - if f.is_file() - ] + artifacts = [f.name for f in input.output_dir.iterdir() if f.is_file()] tool_events = [] if proc.stderr: diff --git a/eval_runner/fake_adapter.py b/eval_runner/fake_adapter.py index 2ba692b..c9621cc 100644 --- a/eval_runner/fake_adapter.py +++ b/eval_runner/fake_adapter.py @@ -28,9 +28,7 @@ class FakeAdapter: start = time.monotonic() case = input.case - response = ( - f"[fake] Processed case '{case.id}': {case.prompt[:80]}" - ) + response = f"[fake] Processed case '{case.id}': {case.prompt[:80]}" activation_evidence = f"skill loaded from {input.skill_path.name}/SKILL.md" tool_events = [ diff --git a/eval_runner/grader.py b/eval_runner/grader.py index b9d9fdd..d128d85 100644 --- a/eval_runner/grader.py +++ b/eval_runner/grader.py @@ -80,7 +80,9 @@ def _check_assertion(assertion: str, output: AdapterOutput) -> AssertionResult: actual = output.exit_status.value if actual == expected: return AssertionResult(assertion, AssertionVerdict.PASS) - return AssertionResult(assertion, AssertionVerdict.FAIL, f"expected {expected}, got {actual}") + return AssertionResult( + assertion, AssertionVerdict.FAIL, f"expected {expected}, got {actual}" + ) if kind == "artifact_exists": if value in output.artifacts: @@ -95,24 +97,36 @@ def _check_assertion(assertion: str, output: AdapterOutput) -> AssertionResult: actual_val = str(output.environment_state[key]) if actual_val == expected_val: return AssertionResult(assertion, AssertionVerdict.PASS) - return AssertionResult(assertion, AssertionVerdict.FAIL, f"{key}={actual_val}, expected {expected_val}") - return AssertionResult(assertion, AssertionVerdict.FAIL, f"key '{key}' not in environment_state") + return AssertionResult( + assertion, AssertionVerdict.FAIL, f"{key}={actual_val}, expected {expected_val}" + ) + return AssertionResult( + assertion, AssertionVerdict.FAIL, f"key '{key}' not in environment_state" + ) if kind == "activation_evidence_contains": if output.activation_evidence and value in output.activation_evidence: return AssertionResult(assertion, AssertionVerdict.PASS) - return AssertionResult(assertion, AssertionVerdict.FAIL, f"'{value}' not in activation_evidence") + return AssertionResult( + assertion, AssertionVerdict.FAIL, f"'{value}' not in activation_evidence" + ) if kind == "tool_event_count_gte": try: threshold = int(value) except ValueError: - return AssertionResult(assertion, AssertionVerdict.MANUAL_REVIEW, "non-integer threshold") + return AssertionResult( + assertion, AssertionVerdict.MANUAL_REVIEW, "non-integer threshold" + ) if len(output.tool_events) >= threshold: return AssertionResult(assertion, AssertionVerdict.PASS) - return AssertionResult(assertion, AssertionVerdict.FAIL, f"{len(output.tool_events)} < {threshold}") + return AssertionResult( + assertion, AssertionVerdict.FAIL, f"{len(output.tool_events)} < {threshold}" + ) - return AssertionResult(assertion, AssertionVerdict.MANUAL_REVIEW, f"unknown assertion kind '{kind}'") + return AssertionResult( + assertion, AssertionVerdict.MANUAL_REVIEW, f"unknown assertion kind '{kind}'" + ) def grade_output(case_id: str, assertions: list[str], output: AdapterOutput) -> GradeResult: @@ -124,11 +138,15 @@ def grade_output(case_id: str, assertions: list[str], output: AdapterOutput) -> """ if output.exit_status != ExitStatus.COMPLETED: results = [ - AssertionResult(a, AssertionVerdict.INFRA_ERROR, f"exit_status={output.exit_status.value}") + AssertionResult( + a, AssertionVerdict.INFRA_ERROR, f"exit_status={output.exit_status.value}" + ) for a in assertions ] return GradeResult(case_id=case_id, passed=False, results=results, infra_error=True) results = [_check_assertion(a, output) for a in assertions] - passed = all(r.verdict in (AssertionVerdict.PASS, AssertionVerdict.MANUAL_REVIEW) for r in results) + passed = all( + r.verdict in (AssertionVerdict.PASS, AssertionVerdict.MANUAL_REVIEW) for r in results + ) return GradeResult(case_id=case_id, passed=passed, results=results) diff --git a/eval_runner/manifest.py b/eval_runner/manifest.py index 1d1d144..1979895 100644 --- a/eval_runner/manifest.py +++ b/eval_runner/manifest.py @@ -5,11 +5,11 @@ from __future__ import annotations import json import subprocess import uuid -from datetime import datetime, timezone +from datetime import datetime from pathlib import Path from typing import Any -from .models import AdapterInput, AdapterOutput, EvalCase +from .models import AdapterInput, AdapterOutput from .path_safety import contained_path, hash_contained_file, validate_case_id MANIFEST_SCHEMA_VERSION = 1 diff --git a/eval_runner/openai_adapter.py b/eval_runner/openai_adapter.py index 215a35c..adc39cd 100644 --- a/eval_runner/openai_adapter.py +++ b/eval_runner/openai_adapter.py @@ -63,17 +63,19 @@ class OpenAICompatAdapter: messages: list[dict[str, str]] = [] skill_content = self._load_skill_content(input.skill_path) if skill_content: - messages.append({ - "role": "system", - "content": ( - "You are an AI assistant with expertise from the following skill. " - "Use the knowledge, frameworks, and methodology described in the skill " - "to answer the user's question directly. Do NOT show commands or scripts " - "to run — instead, apply the framework yourself and provide the answer " - "with your reasoning.\n\n" - f"\n{skill_content}\n" - ), - }) + messages.append( + { + "role": "system", + "content": ( + "You are an AI assistant with expertise from the following skill. " + "Use the knowledge, frameworks, and methodology described in the skill " + "to answer the user's question directly. Do NOT show commands or scripts " + "to run — instead, apply the framework yourself and provide the answer " + "with your reasoning.\n\n" + f"\n{skill_content}\n" + ), + } + ) messages.append({"role": "user", "content": input.case.prompt}) return messages @@ -118,17 +120,17 @@ class OpenAICompatAdapter: tool_events = [] if reasoning: - tool_events.append(ToolEvent( - name="reasoning", - arguments={"model": self._model}, - result_summary=reasoning[:500], - )) + tool_events.append( + ToolEvent( + name="reasoning", + arguments={"model": self._model}, + result_summary=reasoning[:500], + ) + ) skill_content = self._load_skill_content(input.skill_path) activation_evidence = ( - f"skill loaded from {input.skill_path.name}/SKILL.md" - if skill_content - else None + f"skill loaded from {input.skill_path.name}/SKILL.md" if skill_content else None ) return AdapterOutput( diff --git a/eval_runner/paired.py b/eval_runner/paired.py index ecbbb3d..43db275 100644 --- a/eval_runner/paired.py +++ b/eval_runner/paired.py @@ -8,7 +8,6 @@ Mutable state is reset for every trial. from __future__ import annotations -import json import sys from datetime import datetime, timezone from pathlib import Path @@ -165,8 +164,12 @@ def main() -> int: parser.add_argument("--base-url", default=None, help="OpenAI-compatible API base URL") parser.add_argument("--api-key", default=None, help="API key (optional)") parser.add_argument("--max-tokens", type=int, default=4096) - parser.add_argument("--max-skill-chars", type=int, default=None, help="truncate skill content to N chars") - parser.add_argument("--no-thinking", action="store_true", help="disable thinking/reasoning mode (llama.cpp)") + parser.add_argument( + "--max-skill-chars", type=int, default=None, help="truncate skill content to N chars" + ) + parser.add_argument( + "--no-thinking", action="store_true", help="disable thinking/reasoning mode (llama.cpp)" + ) args = parser.parse_args() manifest_path = args.manifest.resolve() @@ -253,7 +256,9 @@ def main() -> int: elif delta == "candidate_regression": regressions += 1 - print(f"summary: {len(reports)} case(s), {improvements} improvement(s), {regressions} regression(s)") + print( + f"summary: {len(reports)} case(s), {improvements} improvement(s), {regressions} regression(s)" + ) return 1 if regressions > 0 else 0 diff --git a/eval_runner/path_safety.py b/eval_runner/path_safety.py index 26be7e7..fbe14c3 100644 --- a/eval_runner/path_safety.py +++ b/eval_runner/path_safety.py @@ -14,11 +14,7 @@ _CASE_ID_RE = re.compile(r"[a-z0-9]+(?:-[a-z0-9]+)*", re.ASCII) def validate_case_id(case_id: str) -> str: """Return a safe case ID or raise ValueError for unsafe input.""" - if ( - not isinstance(case_id, str) - or len(case_id) > 64 - or _CASE_ID_RE.fullmatch(case_id) is None - ): + if not isinstance(case_id, str) or len(case_id) > 64 or _CASE_ID_RE.fullmatch(case_id) is None: raise ValueError( "invalid eval case ID: expected 1-64 lowercase ASCII letters or digits " "separated by single hyphens" diff --git a/eval_runner/release.py b/eval_runner/release.py index c87dea7..0babf9c 100644 --- a/eval_runner/release.py +++ b/eval_runner/release.py @@ -248,7 +248,7 @@ class PairwisePlan: seed: int @property - def reversed(self) -> "PairwisePlan": + def reversed(self) -> PairwisePlan: return PairwisePlan( case_id=self.case_id, position_a=self.position_b, @@ -287,9 +287,12 @@ def evaluate_pairwise( score_a = _mean_score(grade_a) score_b = _mean_score(grade_b) - if grade_a["verdict"] == "abstain" or grade_b["verdict"] == "abstain": - winner = "abstain" - elif grade_a["verdict"] == "insufficient_evidence" or grade_b["verdict"] == "insufficient_evidence": + if ( + grade_a["verdict"] == "abstain" + or grade_b["verdict"] == "abstain" + or grade_a["verdict"] == "insufficient_evidence" + or grade_b["verdict"] == "insufficient_evidence" + ): winner = "abstain" elif abs(score_a - score_b) < 0.05: winner = "tie" @@ -298,7 +301,7 @@ def evaluate_pairwise( else: winner = "b" - reversed_plan = plan.reversed + reversed_plan = plan.reversed # noqa: F841 (used for future blinded grading) grade_rev_a = judge.grade(response_b, expected, blinded=True) grade_rev_b = judge.grade(response_a, expected, blinded=True) score_rev_a = _mean_score(grade_rev_a) @@ -383,13 +386,9 @@ def compute_release_decision( release_cases = [c for c in case_results if c["case_set"] == "release"] regression_cases = [c for c in case_results if c["case_set"] == "regression"] - missing_evidence_cases = [ - c["case_id"] for c in case_results if c.get("missing_evidence") - ] + missing_evidence_cases = [c["case_id"] for c in case_results if c.get("missing_evidence")] if missing_evidence_cases: - reasons.append( - f"missing evidence in cases: {', '.join(missing_evidence_cases)}" - ) + reasons.append(f"missing evidence in cases: {', '.join(missing_evidence_cases)}") return { "outcome": "HOLD", "reasons": reasons, @@ -397,24 +396,16 @@ def compute_release_decision( } inconsistent = [c["case_id"] for c in case_results if not c["consistent"]] - low_frequency = [ - c["case_id"] - for c in release_cases - if c["success_frequency"] < 0.8 - ] + low_frequency = [c["case_id"] for c in release_cases if c["success_frequency"] < 0.8] rubric_abstains = [ - r["case_id"] - for r in rubric_results - if r["verdict"] in ("abstain", "insufficient_evidence") + r["case_id"] for r in rubric_results if r["verdict"] in ("abstain", "insufficient_evidence") ] uncalibrated_advisory = not calibration.calibrated if low_frequency: - reasons.append( - f"release cases below 80% success: {', '.join(low_frequency)}" - ) + reasons.append(f"release cases below 80% success: {', '.join(low_frequency)}") return { "outcome": "BLOCK", "reasons": reasons, @@ -422,9 +413,7 @@ def compute_release_decision( } regressions = [ - c["case_id"] - for c in regression_cases - if c["paired_delta"] == "candidate_regression" + c["case_id"] for c in regression_cases if c["paired_delta"] == "candidate_regression" ] if regressions: reasons.append(f"regressions detected: {', '.join(regressions)}") diff --git a/eval_runner/sandbox.py b/eval_runner/sandbox.py index eb3b3c6..79a35d2 100644 --- a/eval_runner/sandbox.py +++ b/eval_runner/sandbox.py @@ -51,9 +51,7 @@ def _is_excluded(path: Path, skill_root: Path) -> bool: return True if path.name in EXCLUDED_FILES: return True - if path.suffix in EXCLUDED_SUFFIXES: - return True - return False + return path.suffix in EXCLUDED_SUFFIXES def _set_readonly(path: Path) -> None: diff --git a/eval_runner/tests/test_paired.py b/eval_runner/tests/test_paired.py index 243632f..689483f 100644 --- a/eval_runner/tests/test_paired.py +++ b/eval_runner/tests/test_paired.py @@ -10,16 +10,16 @@ from pathlib import Path sys.path.insert(0, str(Path(__file__).resolve().parent.parent.parent)) -from eval_runner.models import AdapterOutput, EvalCase, ExitStatus, ToolEvent -from eval_runner.grader import AssertionVerdict, grade_output -from eval_runner.sandbox import cleanup_sandbox, stage_paired_sandboxes, stage_skill_sandbox from eval_runner.comparison import ( build_comparison_report, format_comparison_summary, write_comparison_report, ) -from eval_runner.paired import run_paired_trial from eval_runner.fake_adapter import FakeAdapter +from eval_runner.grader import AssertionVerdict, grade_output +from eval_runner.models import AdapterOutput, EvalCase, ExitStatus, ToolEvent +from eval_runner.paired import run_paired_trial +from eval_runner.sandbox import cleanup_sandbox, stage_paired_sandboxes, stage_skill_sandbox def _make_skill_dir(tmp: Path) -> Path: @@ -47,7 +47,8 @@ def _make_case(assertions: list[str] | None = None) -> EvalCase: id="paired-test-01", prompt="Do the thing", expected_output="The thing is done", - assertions=assertions or [ + assertions=assertions + or [ "response_contains:paired-test-01", "exit_status:completed", "activation_evidence_contains:test-skill", @@ -80,8 +81,8 @@ def test_sandbox_readonly(): skill_md = staged / "SKILL.md" assert skill_md.is_file() - import os import stat + mode = skill_md.stat().st_mode assert not (mode & stat.S_IWUSR) @@ -108,7 +109,9 @@ def test_grader_pass(): activation_evidence="loaded test-skill/SKILL.md", tool_events=[ToolEvent(name="x")], ) - result = grade_output("c1", ["response_contains:paired-test-01", "exit_status:completed"], output) + result = grade_output( + "c1", ["response_contains:paired-test-01", "exit_status:completed"], output + ) assert result.passed assert result.pass_count == 2 assert result.fail_count == 0 @@ -197,7 +200,9 @@ def test_comparison_report_validates_against_schema(): ) assertions = ["response_contains:paired-test-01"] c_grade = grade_output("c1", assertions, output) - b_grade = grade_output("c1", assertions, AdapterOutput(exit_status=ExitStatus.COMPLETED, response="")) + b_grade = grade_output( + "c1", assertions, AdapterOutput(exit_status=ExitStatus.COMPLETED, response="") + ) report = build_comparison_report( skill_name="test-skill", @@ -401,10 +406,7 @@ def test_cleanup_does_not_follow_replaced_nested_symlink(): def test_workflow_uses_variables_without_deployment_defaults_and_pins_actions(): workflow = ( - Path(__file__).resolve().parent.parent.parent - / ".github" - / "workflows" - / "skill-eval.yml" + Path(__file__).resolve().parent.parent.parent / ".github" / "workflows" / "skill-eval.yml" ).read_text() assert "vars.EVAL_BASE_URL ||" not in workflow assert "vars.EVAL_MODEL ||" not in workflow diff --git a/eval_runner/tests/test_release.py b/eval_runner/tests/test_release.py index 9a27fec..126a33c 100644 --- a/eval_runner/tests/test_release.py +++ b/eval_runner/tests/test_release.py @@ -25,12 +25,16 @@ from eval_runner.release import ( ) -def _trial(case_id: str, status: str = "completed", passed: bool = True, missing: list[str] | None = None) -> dict: +def _trial( + case_id: str, status: str = "completed", passed: bool = True, missing: list[str] | None = None +) -> dict: return { "trial_id": f"t-{case_id}-{status}", "case": {"case_id": case_id, "prompt_hash": "abc", "fixture_hashes": {}}, "status": status, - "failures": [] if passed and status == "completed" else [{"type": "assertion", "message": "fail"}], + "failures": [] + if passed and status == "completed" + else [{"type": "assertion", "message": "fail"}], "missing_evidence": missing or [], } @@ -121,7 +125,9 @@ def test_case_aggregation_to_dict(): def test_rubric_grader_pass(): grader = RubricGrader("test-grader", "1.0", ("relevance", "completeness")) - result = grader.grade("the expected output is here and complete", "expected output is here and complete") + result = grader.grade( + "the expected output is here and complete", "expected output is here and complete" + ) assert result["verdict"] == "pass" assert result["grader_id"] == "test-grader" assert result["grader_version"] == "1.0" @@ -158,7 +164,9 @@ def test_apply_rubric_adds_case_id(): def test_plan_pairwise_deterministic(): plans1 = plan_pairwise(["c1", "c2", "c3"], seed=42) plans2 = plan_pairwise(["c1", "c2", "c3"], seed=42) - assert [(p.case_id, p.position_a) for p in plans1] == [(p.case_id, p.position_a) for p in plans2] + assert [(p.case_id, p.position_a) for p in plans1] == [ + (p.case_id, p.position_a) for p in plans2 + ] def test_plan_pairwise_blinded_positions(): @@ -216,7 +224,14 @@ def test_calibration_low_agreement_not_calibrated(): def test_release_decision_pass(): cases = [ - {"case_id": "c1", "case_set": "release", "success_frequency": 1.0, "consistent": True, "missing_evidence": [], "paired_delta": "both_pass"}, + { + "case_id": "c1", + "case_set": "release", + "success_frequency": 1.0, + "consistent": True, + "missing_evidence": [], + "paired_delta": "both_pass", + }, ] cal = CalibrationRecord(human_sample_count=20, judge_agreement_rate=0.9) decision = compute_release_decision(cases, [], cal) @@ -224,14 +239,23 @@ def test_release_decision_pass(): def test_release_decision_block_on_hard_gate(): - decision = compute_release_decision([], [], CalibrationRecord(), hard_gate_violations=["privacy violation"]) + decision = compute_release_decision( + [], [], CalibrationRecord(), hard_gate_violations=["privacy violation"] + ) assert decision["outcome"] == "BLOCK" assert "privacy violation" in decision["reasons"][0] def test_release_decision_hold_on_missing_evidence(): cases = [ - {"case_id": "c1", "case_set": "release", "success_frequency": 1.0, "consistent": True, "missing_evidence": ["response"], "paired_delta": "both_pass"}, + { + "case_id": "c1", + "case_set": "release", + "success_frequency": 1.0, + "consistent": True, + "missing_evidence": ["response"], + "paired_delta": "both_pass", + }, ] decision = compute_release_decision(cases, [], CalibrationRecord()) assert decision["outcome"] == "HOLD" @@ -239,7 +263,14 @@ def test_release_decision_hold_on_missing_evidence(): def test_release_decision_block_on_low_frequency(): cases = [ - {"case_id": "c1", "case_set": "release", "success_frequency": 0.5, "consistent": False, "missing_evidence": [], "paired_delta": "both_pass"}, + { + "case_id": "c1", + "case_set": "release", + "success_frequency": 0.5, + "consistent": False, + "missing_evidence": [], + "paired_delta": "both_pass", + }, ] cal = CalibrationRecord(human_sample_count=20, judge_agreement_rate=0.9) decision = compute_release_decision(cases, [], cal) @@ -248,7 +279,14 @@ def test_release_decision_block_on_low_frequency(): def test_release_decision_block_on_regression(): cases = [ - {"case_id": "c1", "case_set": "regression", "success_frequency": 1.0, "consistent": True, "missing_evidence": [], "paired_delta": "candidate_regression"}, + { + "case_id": "c1", + "case_set": "regression", + "success_frequency": 1.0, + "consistent": True, + "missing_evidence": [], + "paired_delta": "candidate_regression", + }, ] cal = CalibrationRecord(human_sample_count=20, judge_agreement_rate=0.9) decision = compute_release_decision(cases, [], cal) @@ -257,7 +295,14 @@ def test_release_decision_block_on_regression(): def test_release_decision_conditional_on_inconsistency(): cases = [ - {"case_id": "c1", "case_set": "release", "success_frequency": 0.9, "consistent": False, "missing_evidence": [], "paired_delta": "both_pass"}, + { + "case_id": "c1", + "case_set": "release", + "success_frequency": 0.9, + "consistent": False, + "missing_evidence": [], + "paired_delta": "both_pass", + }, ] cal = CalibrationRecord(human_sample_count=20, judge_agreement_rate=0.9) decision = compute_release_decision(cases, [], cal) @@ -266,7 +311,14 @@ def test_release_decision_conditional_on_inconsistency(): def test_release_decision_conditional_on_uncalibrated_judge(): cases = [ - {"case_id": "c1", "case_set": "release", "success_frequency": 1.0, "consistent": True, "missing_evidence": [], "paired_delta": "both_pass"}, + { + "case_id": "c1", + "case_set": "release", + "success_frequency": 1.0, + "consistent": True, + "missing_evidence": [], + "paired_delta": "both_pass", + }, ] rubric = [{"case_id": "c1", "verdict": "pass"}] cal = CalibrationRecord() @@ -277,7 +329,14 @@ def test_release_decision_conditional_on_uncalibrated_judge(): def test_release_decision_conditional_on_rubric_abstain(): cases = [ - {"case_id": "c1", "case_set": "release", "success_frequency": 1.0, "consistent": True, "missing_evidence": [], "paired_delta": "both_pass"}, + { + "case_id": "c1", + "case_set": "release", + "success_frequency": 1.0, + "consistent": True, + "missing_evidence": [], + "paired_delta": "both_pass", + }, ] rubric = [{"case_id": "c1", "verdict": "abstain"}] cal = CalibrationRecord(human_sample_count=20, judge_agreement_rate=0.9) @@ -336,9 +395,7 @@ def test_build_release_report_validates_against_schema(): return schema_path = ( - Path(__file__).resolve().parent.parent.parent - / "schemas" - / "release-eval-v1.schema.json" + Path(__file__).resolve().parent.parent.parent / "schemas" / "release-eval-v1.schema.json" ) schema = json.loads(schema_path.read_text()) Draft202012Validator.check_schema(schema) @@ -404,9 +461,7 @@ def test_release_schema_matches_runtime_case_ids(): return schema_path = ( - Path(__file__).resolve().parent.parent.parent - / "schemas" - / "release-eval-v1.schema.json" + Path(__file__).resolve().parent.parent.parent / "schemas" / "release-eval-v1.schema.json" ) schema = json.loads(schema_path.read_text()) for definition in ["case_result", "rubric_result", "pairwise_result"]: diff --git a/eval_runner/tests/test_runner.py b/eval_runner/tests/test_runner.py index c5a8d97..16ee7d3 100644 --- a/eval_runner/tests/test_runner.py +++ b/eval_runner/tests/test_runner.py @@ -5,13 +5,14 @@ from __future__ import annotations import json import sys import tempfile +from datetime import datetime, timezone from pathlib import Path sys.path.insert(0, str(Path(__file__).resolve().parent.parent.parent)) -from eval_runner import models as models from eval_runner import fake_adapter as fake_adapter_mod from eval_runner import manifest as manifest_mod +from eval_runner import models as models from eval_runner import path_safety as path_safety_mod from eval_runner.path_safety import contained_path from eval_runner.runner import load_cases @@ -24,8 +25,6 @@ FakeAdapter = fake_adapter_mod.FakeAdapter build_manifest = manifest_mod.build_manifest write_manifest = manifest_mod.write_manifest -from datetime import datetime, timezone - def _make_case() -> EvalCase: return EvalCase( @@ -150,9 +149,7 @@ def test_manifest_validates_against_schema(): return schema_path = ( - Path(__file__).resolve().parent.parent.parent - / "schemas" - / "run-manifest-v1.schema.json" + Path(__file__).resolve().parent.parent.parent / "schemas" / "run-manifest-v1.schema.json" ) schema = json.loads(schema_path.read_text()) Draft202012Validator.check_schema(schema) @@ -200,9 +197,7 @@ def test_manifest_schema_enforces_public_identity_fields(): return schema_path = ( - Path(__file__).resolve().parent.parent.parent - / "schemas" - / "run-manifest-v1.schema.json" + Path(__file__).resolve().parent.parent.parent / "schemas" / "run-manifest-v1.schema.json" ) schema = json.loads(schema_path.read_text()) candidate_properties = schema["properties"]["candidate"]["properties"] @@ -255,12 +250,15 @@ def test_eval_case_rejects_unsafe_ids(): else: raise AssertionError(f"unsafe case ID accepted: {case_id!r}") - assert EvalCase( - id="x" * 64, - prompt="prompt", - expected_output="output", - assertions=[], - ).id == "x" * 64 + assert ( + EvalCase( + id="x" * 64, + prompt="prompt", + expected_output="output", + assertions=[], + ).id + == "x" * 64 + ) def test_eval_case_rejects_unsafe_fixture_paths(): diff --git a/pyproject.toml b/pyproject.toml new file mode 100644 index 0000000..a456591 --- /dev/null +++ b/pyproject.toml @@ -0,0 +1,72 @@ +[tool.ruff] +target-version = "py310" +line-length = 100 + +[tool.ruff.lint] +select = [ + "E", # pycodestyle errors + "F", # pyflakes (unused imports, variables) + "W", # pycodestyle warnings + "I", # isort (import order) + "N", # pep8-naming (naming conventions) + "UP", # pyupgrade (modern Python syntax) + "B", # flake8-bugbear (common bug patterns) + "C4", # flake8-comprehensions + "SIM", # flake8-simplify + "RUF", # ruff-specific rules +] +ignore = [ + "E501", # line too long (handled by formatter) + "N818", # exception class names (not every exception needs "Error" suffix) + "N802", # function names (test methods and templates use non-snake-case) + "N806", # variable names (templates use non-snake-case) +] + +[tool.ruff.lint.per-file-ignores] +# Templates and generated code +"**/templates/*.py" = ["N802", "N803", "N806", "E402"] +# CLI entry points may shadow builtins intentionally +"**/cli.py" = ["A001", "A002"] +# eval_runner modules with deliberate import patterns +"eval_runner/comparison.py" = ["TCH"] +"eval_runner/adapter.py" = ["TCH"] +"eval_runner/manifest.py" = ["E402"] + +[tool.ruff.format] +quote-style = "double" +indent-style = "space" +skip-magic-trailing-comma = false +line-ending = "auto" + +[tool.pytest.ini_options] +minversion = "7.0" +testpaths = ["scripts", "eval_runner/tests"] +python_files = "test_*.py" +python_classes = "*Test*" +python_functions = "test_*" +addopts = [ + "-ra", + "--strict-markers", + "--tb=short", + "--durations=10", + "--cov=scripts", + "--cov=eval_runner", + "--cov-report=term-missing", + "--cov-fail-under=60", +] + +[tool.coverage.run] +branch = true +source = ["scripts", "eval_runner"] +omit = [ + "*/tests/*", + "*/templates/*", + "*/test_*.py", +] + +[tool.coverage.report] +exclude_lines = [ + "pragma: no cover", + "if __name__ == .__main__.:", + "raise NotImplementedError", +] diff --git a/requirements-dev.txt b/requirements-dev.txt index 66cc01a..c5a708e 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,2 +1,5 @@ jsonschema[format]==4.26.0 requests==2.34.2 +ruff>=0.9.0 +pytest>=7.0 +pytest-cov>=4.0 diff --git a/scripts/check-artifacts.py b/scripts/check-artifacts.py index 381b072..43bc337 100644 --- a/scripts/check-artifacts.py +++ b/scripts/check-artifacts.py @@ -8,7 +8,6 @@ import sys import unittest from pathlib import Path - ROOT = Path(__file__).resolve().parent.parent @@ -70,9 +69,7 @@ def run_checks(files: list[Path]) -> list[str]: for directory in test_directories(files): relative = directory.relative_to(ROOT) result = unittest.TextTestRunner(verbosity=0).run( - unittest.defaultTestLoader.discover( - str(directory), top_level_dir=str(directory) - ) + unittest.defaultTestLoader.discover(str(directory), top_level_dir=str(directory)) ) if not result.wasSuccessful(): errors.append( diff --git a/scripts/eval-coverage.py b/scripts/eval-coverage.py index 7915f46..00a77e7 100644 --- a/scripts/eval-coverage.py +++ b/scripts/eval-coverage.py @@ -24,8 +24,8 @@ from eval_validation import NOT_ASSESSED, STATE_NAMES, ValidationResult, validat ROOT = Path(__file__).resolve().parent.parent # Phase 3 ratchet thresholds (percent of skills with evals) -WARN_THRESHOLD = 25 # modified skills without evals get a warning -FAIL_THRESHOLD = 50 # modified skills without evals fail CI +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 @@ -64,9 +64,7 @@ def resolve_ref_to_commit(ref: str) -> str: text=True, ) if result.returncode != 0: - raise ValueError( - f"invalid --modified-from ref: {ref!r} is not an existing commit" - ) + raise ValueError(f"invalid --modified-from ref: {ref!r} is not an existing commit") return result.stdout.strip() @@ -81,13 +79,11 @@ def find_skills_at(ref: str) -> list[Path]: ) skills = [] for name in result.stdout.splitlines(): - if ( - name.endswith("/SKILL.md") - and "/agent-council/profiles/skills/" not in name - ): + if name.endswith("/SKILL.md") and "/agent-council/profiles/skills/" not in name: skills.append(Path(name).parent) return sorted(skills) + def check_eval_states(skill_dir: Path) -> ValidationResult: """Return validation and evidence states for one skill.""" evals_file = ROOT / skill_dir / "evals" / "evals.json" @@ -129,7 +125,8 @@ def modified_skills(base_ref_commit: str) -> set[Path]: text=True, ) changed_files = [ - line for line in result.stdout.strip().splitlines() + 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 @@ -206,7 +203,9 @@ def coverage_decreased(base_ref_commit: str) -> tuple[bool, float, float]: tar.extractall(snapshot, filter="data") subprocess.run(["git", "init", "-q"], cwd=snapshot, check=True) subprocess.run( - ["git", "add", "-f", "--all"], cwd=snapshot, check=True, + ["git", "add", "-f", "--all"], + cwd=snapshot, + check=True, capture_output=True, ) for skill_dir in retained_skill_dirs: @@ -290,9 +289,7 @@ def main() -> int: coverage_pct = state_summary["schema_valid"]["percentage"] # Sort skills without evals: most-referenced first, then alphabetical - ref_counts = { - name: count_references(Path(name).name, skills) for name in without_evals - } + ref_counts = {name: count_references(Path(name).name, skills) for name in without_evals} without_evals.sort(key=lambda n: (-ref_counts[n], n)) # Phase 3 ratchet check @@ -338,21 +335,19 @@ def main() -> int: summary = state_summary[state] if summary["assessment"] == "supported": print( - f" {state}: {summary['count']}/{total} skills " - f"({summary['percentage']:.1f}%)" + f" {state}: {summary['count']}/{total} skills ({summary['percentage']:.1f}%)" ) continue print(f" {state}: not assessed ({summary['reason']})") print() - print(f"Skills WITHOUT schema-valid eval manifests ({len(without_evals)}), by reference count:") + print( + f"Skills WITHOUT schema-valid eval manifests ({len(without_evals)}), by reference count:" + ) for name in without_evals: refs = ref_counts.get(name, 0) print(f" - {name} (referenced by {refs} skills)") print() - print( - f"Ratchet: warn at {WARN_THRESHOLD}%, " - f"fail-on-modify at {FAIL_THRESHOLD}%" - ) + print(f"Ratchet: warn at {WARN_THRESHOLD}%, fail-on-modify at {FAIL_THRESHOLD}%") if ratchet_warnings: print() print("Ratchet warnings:") diff --git a/scripts/eval_validation.py b/scripts/eval_validation.py index b4d0b1e..2a039e4 100644 --- a/scripts/eval_validation.py +++ b/scripts/eval_validation.py @@ -87,11 +87,7 @@ def _tracked_files(repo_root: Path) -> set[str]: result = _git(repo_root, "ls-files", "-z") if result.returncode != 0: return set() - return { - path - for path in result.stdout.decode("utf-8", errors="replace").split("\0") - if path - } + return {path for path in result.stdout.decode("utf-8", errors="replace").split("\0") if path} def _tracked_mode(repo_root: Path, relative_path: str) -> str | None: @@ -131,7 +127,7 @@ def _targeted_schema_version_errors(data: dict[str, Any], result: ValidationResu if "schema_version" not in data: result.error( "$.schema_version", - "missing required schema_version; add \"schema_version\": 1", + 'missing required schema_version; add "schema_version": 1', ) return version = data["schema_version"] diff --git a/scripts/test-eval-coverage.py b/scripts/test-eval-coverage.py index 7d156a5..511d9d6 100644 --- a/scripts/test-eval-coverage.py +++ b/scripts/test-eval-coverage.py @@ -5,9 +5,11 @@ Uses a temporary git repository to verify modified-skill detection, coverage-decrease detection, and threshold behavior. """ -import json +# 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 import io -import os +import json import shutil import subprocess import sys @@ -17,15 +19,10 @@ from contextlib import redirect_stderr, redirect_stdout from pathlib import Path from unittest import mock -# 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 -from eval_validation import NOT_APPLICABLE # noqa: E402 +from eval_validation import NOT_APPLICABLE SCRIPT_DIR = Path(__file__).resolve().parent -_spec = importlib.util.spec_from_file_location( - "eval_coverage", SCRIPT_DIR / "eval-coverage.py" -) +_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] @@ -159,9 +156,7 @@ class TestModifiedSkills(unittest.TestCase): try: modified = eval_coverage.modified_skills(base) current = set(eval_coverage.find_skills()) - without_evals = { - skill for skill in current if not eval_coverage.check_evals(skill)[0] - } + without_evals = {skill for skill in current if not eval_coverage.check_evals(skill)[0]} warnings, errors = eval_coverage.evaluate_ratchet( modified=modified, current=current, @@ -419,9 +414,12 @@ class TestBaseRefValidation(unittest.TestCase): eval_coverage.ROOT = Path(self.repo) stderr = io.StringIO() try: - with mock.patch.object( - sys, "argv", ["eval-coverage.py", "--modified-from", "does-not-exist"] - ), redirect_stderr(stderr): + with ( + mock.patch.object( + sys, "argv", ["eval-coverage.py", "--modified-from", "does-not-exist"] + ), + redirect_stderr(stderr), + ): exit_code = eval_coverage.main() finally: eval_coverage.ROOT = old_root @@ -436,9 +434,10 @@ class TestBaseRefValidation(unittest.TestCase): eval_coverage.ROOT = Path(self.repo) stderr = io.StringIO() try: - with mock.patch.object( - sys, "argv", ["eval-coverage.py", "--modified-from=--name-only"] - ), redirect_stderr(stderr): + with ( + mock.patch.object(sys, "argv", ["eval-coverage.py", "--modified-from=--name-only"]), + redirect_stderr(stderr), + ): exit_code = eval_coverage.main() finally: eval_coverage.ROOT = old_root @@ -453,9 +452,13 @@ class TestBaseRefValidation(unittest.TestCase): eval_coverage.ROOT = Path(self.repo) stderr = io.StringIO() try: - with mock.patch.object( - sys, "argv", ["eval-coverage.py", "--modified-from", "main", "--json"] - ), redirect_stderr(stderr), redirect_stdout(io.StringIO()): + with ( + mock.patch.object( + sys, "argv", ["eval-coverage.py", "--modified-from", "main", "--json"] + ), + redirect_stderr(stderr), + redirect_stdout(io.StringIO()), + ): exit_code = eval_coverage.main() finally: eval_coverage.ROOT = old_root @@ -480,7 +483,10 @@ class TestCoverageOutput(unittest.TestCase): eval_coverage.ROOT = Path(self.repo) output = io.StringIO() try: - with mock.patch.object(sys, "argv", ["eval-coverage.py", "--json"]), redirect_stdout(output): + with ( + mock.patch.object(sys, "argv", ["eval-coverage.py", "--json"]), + redirect_stdout(output), + ): exit_code = eval_coverage.main() finally: eval_coverage.ROOT = old_root diff --git a/scripts/test-eval-validation.py b/scripts/test-eval-validation.py index c77e9fe..e6fcdc3 100644 --- a/scripts/test-eval-validation.py +++ b/scripts/test-eval-validation.py @@ -12,7 +12,6 @@ from pathlib import Path from eval_validation import NOT_APPLICABLE, SCHEMA_VERSION, find_skill_manifests, validate_manifest - SCRIPT_DIR = Path(__file__).resolve().parent ROOT = SCRIPT_DIR.parent @@ -52,7 +51,9 @@ class EvalValidationTest(unittest.TestCase): git(self.root, "config", "user.name", "Test") self.skill = self.root / "example" (self.skill / "evals").mkdir(parents=True) - (self.skill / "SKILL.md").write_text("---\nname: example\ndescription: test\n---\n", encoding="utf-8") + (self.skill / "SKILL.md").write_text( + "---\nname: example\ndescription: test\n---\n", encoding="utf-8" + ) self.manifest = self.skill / "evals" / "evals.json" def tearDown(self) -> None: @@ -163,17 +164,25 @@ class EvalValidationTest(unittest.TestCase): def test_unknown_top_level_and_case_fields_fail(self) -> None: top_level_errors = self.errors(self.valid(unknown_field=True)) self.assertTrue( - any("Additional properties" in error and "unknown_field" in error for error in top_level_errors) + any( + "Additional properties" in error and "unknown_field" in error + for error in top_level_errors + ) ) case_errors = self.errors(self.valid(evals=[eval_case(unknown_field=True)])) self.assertTrue( - any("Additional properties" in error and "unknown_field" in error for error in case_errors) + any( + "Additional properties" in error and "unknown_field" in error + for error in case_errors + ) ) def test_evidence_property_is_rejected_as_unknown_v1_property(self) -> None: errors = self.errors(self.valid(evidence={"grader_bindings": ["scripts/grader.py"]})) - self.assertTrue(any("Additional properties" in error and "evidence" in error for error in errors)) + self.assertTrue( + any("Additional properties" in error and "evidence" in error for error in errors) + ) def test_duplicate_json_object_keys_fail_before_semantic_validation(self) -> None: self.manifest.write_text( @@ -261,7 +270,9 @@ class EvalValidationTest(unittest.TestCase): os.symlink(external_dir, fixtures_dir / "external") git(self.root, "add", "-A") - errors = self.errors(self.valid(evals=[eval_case(files=["fixtures/external/outside.json"])])) + errors = self.errors( + self.valid(evals=[eval_case(files=["fixtures/external/outside.json"])]) + ) self.assertTrue(any("symlink" in error for error in errors)) def test_tracked_but_missing_file_fails_when_index_still_lists_it(self) -> None: @@ -347,7 +358,9 @@ class EvalValidationTest(unittest.TestCase): self.assertTrue(any("does not exist" in error for error in missing_errors)) self.assertFalse(any("not tracked by Git" in error for error in missing_errors)) - self.write(self.valid(evals=[eval_case(files=["fixtures/untracked.json"])]), track_all=False) + self.write( + self.valid(evals=[eval_case(files=["fixtures/untracked.json"])]), track_all=False + ) git(self.root, "add", str(self.manifest.relative_to(self.root))) tracked_untracked = git_output( self.root,