diff --git a/.github/workflows/validate-task.yml b/.github/workflows/validate-task.yml index 8557f426..1c0faf4f 100644 --- a/.github/workflows/validate-task.yml +++ b/.github/workflows/validate-task.yml @@ -5,15 +5,17 @@ on: branches: [main] paths: - ".github/workflows/validate-task.yml" + - "scripts/ci/validate_tasks.py" - "test-cases/task.schema.json" - - "test-cases/**/task.json" + - "test-cases/**/*.json" - "test-cases/**/extra_info/**" push: branches: [main] paths: - ".github/workflows/validate-task.yml" + - "scripts/ci/validate_tasks.py" - "test-cases/task.schema.json" - - "test-cases/**/task.json" + - "test-cases/**/*.json" - "test-cases/**/extra_info/**" workflow_dispatch: @@ -40,15 +42,15 @@ jobs: run: | set -euo pipefail - if [[ "${{ github.event_name }}" == "workflow_dispatch" ]]; then - find test-cases -path "*/task.json" -o -path "*/extra_info/*" > changed-files.txt - elif [[ "${{ github.event_name }}" == "pull_request" ]]; then + if [[ "${{ github.event_name }}" == "pull_request" ]]; then base="${{ github.event.pull_request.base.sha }}" git diff --name-only "$base"...HEAD > changed-files.txt elif [[ "${{ github.event_name }}" == "push" && "${{ github.event.before }}" != "0000000000000000000000000000000000000000" ]]; then git diff --name-only "${{ github.event.before }}" "${{ github.sha }}" > changed-files.txt else - find test-cases -path "*/task.json" -o -path "*/extra_info/*" > changed-files.txt + # workflow_dispatch, or a push with no usable base: check everything. + : > changed-files.txt + echo "validate_all=true" >> "$GITHUB_OUTPUT" fi echo "Changed files:" @@ -56,83 +58,9 @@ jobs: - name: Validate changed tasks run: | - python - <<'PY' - import json - import sys - from pathlib import Path - - from jsonschema import Draft202012Validator - - repo = Path(".") - schema_path = repo / "test-cases" / "task.schema.json" - changed_paths = [ - Path(line.strip()) - for line in Path("changed-files.txt").read_text().splitlines() - if line.strip() - ] - - schema = json.loads(schema_path.read_text()) - validator = Draft202012Validator(schema) - - validate_all = schema_path in changed_paths - task_files: set[Path] = set() - changed_json_files: set[Path] = set() - - if validate_all: - task_files.update(repo.glob("test-cases/**/task.json")) - - for path in changed_paths: - if not str(path).startswith("test-cases/"): - continue - if path.name == "task.json" and path.exists(): - task_files.add(path) - changed_json_files.add(path) - continue - if "extra_info" in path.parts: - extra_index = path.parts.index("extra_info") - task_dir = Path(*path.parts[:extra_index]) - task_file = task_dir / "task.json" - if task_file.exists(): - task_files.add(task_file) - if path.suffix == ".json" and path.exists(): - changed_json_files.add(path) - - errors: list[str] = [] - - for json_file in sorted(changed_json_files): - try: - json.loads(json_file.read_text()) - except Exception as exc: - errors.append(f"{json_file}: invalid JSON: {exc}") - - for task_file in sorted(task_files): - try: - task = json.loads(task_file.read_text()) - except Exception as exc: - errors.append(f"{task_file}: invalid JSON: {exc}") - continue - - for error in sorted(validator.iter_errors(task), key=lambda item: list(item.path)): - location = "/" + "/".join(str(part) for part in error.path) - errors.append(f"{task_file}{location}: {error.message}") - - extra_info = task.get("extra_info") or [] - if not isinstance(extra_info, list): - continue - for index, item in enumerate(extra_info): - if not isinstance(item, dict) or not item.get("path"): - continue - extra_path = task_file.parent / item["path"] - if not extra_path.exists(): - errors.append( - f"{task_file}: extra_info[{index}].path does not exist: {item['path']}" - ) - - if errors: - print("Task validation failed:") - for error in errors: - print(f"- {error}") - sys.exit(1) - - print(f"Validated {len(task_files)} task file(s) and {len(changed_json_files)} changed JSON file(s).") - PY + if [[ "${{ steps.changed.outputs.validate_all }}" == "true" ]]; then + python scripts/ci/validate_tasks.py --all + else + python scripts/ci/validate_tasks.py --changed-files changed-files.txt + fi + shell: bash diff --git a/pyproject.toml b/pyproject.toml index 2b6a4d4c..e3b56091 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -70,6 +70,10 @@ target-version = "py311" [tool.pyright] include = ["src/clawbench", "tests"] +# tests/test_validate_tasks_script.py imports the CI task validator, which +# is a standalone script rather than part of the package. Setting +# extraPaths replaces the implicit source root, so "src" is listed too. +extraPaths = ["src", "scripts/ci"] exclude = [ ".venv", "src/clawbench/runtime", diff --git a/scripts/ci/validate_tasks.py b/scripts/ci/validate_tasks.py new file mode 100644 index 00000000..156230a2 --- /dev/null +++ b/scripts/ci/validate_tasks.py @@ -0,0 +1,173 @@ +#!/usr/bin/env python3 +"""Validate ClawBench task files against test-cases/task.schema.json. + +Two corpus layouts exist and both are first-class: + +- v1 / v2 / v1-lite keep one directory per task, holding a `task.json`. +- claw-eval keeps one flat `/.json` per task. + +A validator that only knows about `task.json` silently ignores the whole +claw-eval suite, which is registered in `CASE_SUITES` and offered in the TUI, +so a malformed task there surfaces at run time instead of at review time. + +Lives in a script rather than inline in the workflow so the collection rules +can be tested; that is what let the gap go unnoticed in the first place. +""" + +from __future__ import annotations + +import argparse +import json +import sys +from pathlib import Path + +from jsonschema import Draft202012Validator + +TEST_CASES = "test-cases" +SCHEMA_PATH = Path(TEST_CASES) / "task.schema.json" + +# JSON under test-cases/ that is not a task document. +NOT_TASKS = {"task.schema.json", "eligibility-report.json"} + + +def is_nested_task(path: Path) -> bool: + """`test-cases///task.json` — the v1/v2/v1-lite layout.""" + return path.parts[:1] == (TEST_CASES,) and path.name == "task.json" + + +def is_flat_task(path: Path) -> bool: + """`test-cases//.json` — the claw-eval layout.""" + return ( + path.parts[:1] == (TEST_CASES,) + and len(path.parts) == 3 + and path.suffix == ".json" + and path.name not in NOT_TASKS + and "extra_info" not in path.parts + ) + + +def is_task_file(path: Path) -> bool: + return is_nested_task(path) or is_flat_task(path) + + +def all_task_files(repo: Path) -> set[Path]: + """Every task document in the corpus, in either layout.""" + found = {p.relative_to(repo) for p in repo.glob(f"{TEST_CASES}/**/task.json")} + found |= { + p.relative_to(repo) + for p in repo.glob(f"{TEST_CASES}/*/*.json") + if is_flat_task(p.relative_to(repo)) + } + return found + + +def collect(changed_paths: list[Path], repo: Path) -> tuple[set[Path], set[Path]]: + """Return (task files to validate, changed JSON files to parse-check). + + A change to the schema itself re-validates the whole corpus; otherwise only + what the diff touched, plus the owning task of any changed `extra_info`. + """ + task_files: set[Path] = set() + changed_json: set[Path] = set() + + if SCHEMA_PATH in changed_paths: + task_files |= all_task_files(repo) + + for path in changed_paths: + if path.parts[:1] != (TEST_CASES,): + continue + if is_task_file(path) and (repo / path).exists(): + task_files.add(path) + changed_json.add(path) + continue + if "extra_info" in path.parts: + extra_index = path.parts.index("extra_info") + owner = Path(*path.parts[:extra_index]) / "task.json" + if (repo / owner).exists(): + task_files.add(owner) + if path.suffix == ".json" and (repo / path).exists(): + changed_json.add(path) + + return task_files, changed_json + + +def validate(task_files: set[Path], changed_json: set[Path], repo: Path) -> list[str]: + schema = json.loads((repo / SCHEMA_PATH).read_text(encoding="utf-8")) + validator = Draft202012Validator(schema) + errors: list[str] = [] + + for json_file in sorted(changed_json): + try: + json.loads((repo / json_file).read_text(encoding="utf-8")) + except Exception as exc: # noqa: BLE001 — reported, not handled + errors.append(f"{json_file}: invalid JSON: {exc}") + + for task_file in sorted(task_files): + try: + task = json.loads((repo / task_file).read_text(encoding="utf-8")) + except Exception as exc: # noqa: BLE001 — reported, not handled + errors.append(f"{task_file}: invalid JSON: {exc}") + continue + + for error in sorted(validator.iter_errors(task), key=lambda e: list(e.path)): + location = "/" + "/".join(str(part) for part in error.path) + errors.append(f"{task_file}{location}: {error.message}") + + extra_info = task.get("extra_info") or [] + if not isinstance(extra_info, list): + continue + for index, item in enumerate(extra_info): + if not isinstance(item, dict) or not item.get("path"): + continue + if not (repo / task_file.parent / item["path"]).exists(): + errors.append( + f"{task_file}: extra_info[{index}].path does not exist: " + f"{item['path']}" + ) + + return errors + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--changed-files", + type=Path, + help="File listing changed paths, one per line (from git diff --name-only).", + ) + parser.add_argument( + "--all", + action="store_true", + help="Validate the whole corpus regardless of what changed.", + ) + parser.add_argument("--repo", type=Path, default=Path(".")) + args = parser.parse_args(argv) + + repo = args.repo + if args.all or not args.changed_files: + task_files = all_task_files(repo) + changed_json: set[Path] = set() + else: + changed_paths = [ + Path(line.strip()) + for line in args.changed_files.read_text(encoding="utf-8").splitlines() + if line.strip() + ] + task_files, changed_json = collect(changed_paths, repo) + + errors = validate(task_files, changed_json, repo) + if errors: + print("Task validation failed:") + for error in errors: + print(f"- {error}") + return 1 + + print( + f"Validated {len(task_files)} task file(s) and " + f"{len(changed_json)} changed JSON file(s)." + ) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_validate_tasks_script.py b/tests/test_validate_tasks_script.py new file mode 100644 index 00000000..aeec3de6 --- /dev/null +++ b/tests/test_validate_tasks_script.py @@ -0,0 +1,205 @@ +"""CI must see both corpus layouts, not just //task.json. + +The claw-eval suite is a first-class `--cases-suite` target and is offered in +the TUI, but it stores one flat `/.json` per task. The validate-task +workflow's path filter (`test-cases/**/task.json`) never matched those files, +so a PR touching a claw-eval task was merged with no schema validation at all. + +Fixing the trigger alone was not enough: the collector only recognised files +named `task.json`, so even once the workflow fired it validated nothing from +that suite. Both halves are covered here. +""" + +from __future__ import annotations + +import json +import sys +from pathlib import Path + +import pytest +import yaml + +REPO_ROOT = Path(__file__).resolve().parents[1] +WORKFLOW = REPO_ROOT / ".github" / "workflows" / "validate-task.yml" + +sys.path.insert(0, str(REPO_ROOT / "scripts" / "ci")) + +import validate_tasks as vt # noqa: E402 + + +# --- the layouts the collector must recognise --------------------------------- + + +@pytest.mark.parametrize( + "path", + [ + "test-cases/claw-eval/ce-T046-cve-research.json", + "test-cases/claw-eval/ce-T045zh-cve-research.json", + ], +) +def test_a_flat_suite_file_is_a_task(path: str) -> None: + assert vt.is_flat_task(Path(path)) + assert vt.is_task_file(Path(path)) + + +def test_a_directory_style_task_is_still_a_task() -> None: + path = Path("test-cases/v2/002-daily-life-food-doordash/task.json") + assert vt.is_nested_task(path) + assert vt.is_task_file(path) + assert not vt.is_flat_task(path) + + +@pytest.mark.parametrize( + "path", + [ + "test-cases/task.schema.json", # the schema, not a task + "test-cases/claw-eval/eligibility-report.json", # a report + "test-cases/v2/002-x/extra_info/menu.json", # task payload, not a task + "test-cases/v2/002-x/task.md", # not JSON + "docs/scoring.md", # outside the corpus + ], +) +def test_non_task_json_is_not_collected_as_a_task(path: str) -> None: + """Broadening the trigger to *.json must not turn every file into a task.""" + assert not vt.is_task_file(Path(path)) + + +def test_the_flat_predicate_matches_the_runners_own_rule() -> None: + """batch._flat_case_files and tests/test_host_tasks both treat every + /*.json except eligibility-report.json as a case; the validator must + agree, or CI checks a different set of files than the runner executes.""" + from clawbench.runner.batch import _flat_case_files + + base = REPO_ROOT / "test-cases" / "claw-eval" + runner_view = {p.name for p in _flat_case_files(base)} + validator_view = { + p.name + for p in base.glob("*.json") + if vt.is_flat_task(Path("test-cases/claw-eval") / p.name) + } + + assert runner_view == validator_view + assert runner_view, "claw-eval should not be empty" + + +# --- collection --------------------------------------------------------------- + + +def test_a_changed_flat_task_is_validated(tmp_path: Path) -> None: + """The regression: this file used to be collected by nothing.""" + suite = tmp_path / "test-cases" / "claw-eval" + suite.mkdir(parents=True) + (suite / "ce-T046.json").write_text("{}") + + task_files, changed_json = vt.collect( + [Path("test-cases/claw-eval/ce-T046.json")], tmp_path + ) + + assert task_files == {Path("test-cases/claw-eval/ce-T046.json")} + assert changed_json == task_files + + +def test_a_changed_nested_task_is_validated(tmp_path: Path) -> None: + case = tmp_path / "test-cases" / "v2" / "002-x" + case.mkdir(parents=True) + (case / "task.json").write_text("{}") + + task_files, _ = vt.collect([Path("test-cases/v2/002-x/task.json")], tmp_path) + + assert task_files == {Path("test-cases/v2/002-x/task.json")} + + +def test_changed_extra_info_pulls_in_its_owning_task(tmp_path: Path) -> None: + case = tmp_path / "test-cases" / "v2" / "002-x" + (case / "extra_info").mkdir(parents=True) + (case / "task.json").write_text("{}") + (case / "extra_info" / "menu.json").write_text("{}") + + task_files, changed_json = vt.collect( + [Path("test-cases/v2/002-x/extra_info/menu.json")], tmp_path + ) + + assert task_files == {Path("test-cases/v2/002-x/task.json")} + assert changed_json == {Path("test-cases/v2/002-x/extra_info/menu.json")} + + +def test_a_schema_change_revalidates_both_layouts(tmp_path: Path) -> None: + """A schema edit has to re-check the flat suite too, or claw-eval drifts + out of conformance the moment the schema tightens.""" + root = tmp_path / "test-cases" + (root / "v2" / "002-x").mkdir(parents=True) + (root / "claw-eval").mkdir(parents=True) + (root / "v2" / "002-x" / "task.json").write_text("{}") + (root / "claw-eval" / "ce-T046.json").write_text("{}") + (root / "task.schema.json").write_text("{}") + + task_files, _ = vt.collect([vt.SCHEMA_PATH], tmp_path) + + assert task_files == { + Path("test-cases/v2/002-x/task.json"), + Path("test-cases/claw-eval/ce-T046.json"), + } + + +def test_a_deleted_task_is_not_validated(tmp_path: Path) -> None: + """git diff lists deletions; there is nothing left to read.""" + (tmp_path / "test-cases" / "claw-eval").mkdir(parents=True) + + task_files, changed_json = vt.collect( + [Path("test-cases/claw-eval/gone.json")], tmp_path + ) + + assert task_files == set() + assert changed_json == set() + + +# --- validation --------------------------------------------------------------- + + +def test_a_malformed_flat_task_is_reported(tmp_path: Path) -> None: + """The point of the whole workflow: this must fail review, not run time.""" + root = tmp_path / "test-cases" + (root / "claw-eval").mkdir(parents=True) + (root / "task.schema.json").write_text( + json.dumps({"type": "object", "required": ["instruction"]}) + ) + bad = root / "claw-eval" / "ce-T046.json" + bad.write_text(json.dumps({"time_limit": 10})) + + errors = vt.validate({Path("test-cases/claw-eval/ce-T046.json")}, set(), tmp_path) + + assert len(errors) == 1 + assert "instruction" in errors[0] + + +def test_the_real_claw_eval_suite_validates() -> None: + """Turning the check on must not immediately break CI.""" + flat = {p for p in vt.all_task_files(REPO_ROOT) if vt.is_flat_task(p)} + + assert len(flat) == 19, flat + assert vt.validate(flat, set(), REPO_ROOT) == [] + + +# --- the workflow trigger ----------------------------------------------------- + + +def _trigger_paths(event: str) -> list[str]: + workflow = yaml.safe_load(WORKFLOW.read_text(encoding="utf-8")) + # "on" is parsed as the boolean True by YAML 1.1. + triggers = workflow.get("on") or workflow[True] + return triggers[event]["paths"] + + +@pytest.mark.parametrize("event", ["pull_request", "push"]) +def test_the_workflow_fires_for_flat_suite_files(event: str) -> None: + """The original bug was entirely in this filter: `test-cases/**/task.json` + cannot match `test-cases/claw-eval/ce-T046-cve-research.json`.""" + paths = _trigger_paths(event) + + assert "test-cases/**/*.json" in paths + assert "test-cases/**/task.json" not in paths + + +@pytest.mark.parametrize("event", ["pull_request", "push"]) +def test_the_workflow_reruns_when_the_validator_itself_changes(event: str) -> None: + assert "scripts/ci/validate_tasks.py" in _trigger_paths(event)