diff --git a/backend/tests/test_review_changed_public_skills.py b/backend/tests/test_review_changed_public_skills.py index 5b2ef16f46c..9c199d5c82c 100644 --- a/backend/tests/test_review_changed_public_skills.py +++ b/backend/tests/test_review_changed_public_skills.py @@ -38,6 +38,8 @@ def fail_review(*args, **kwargs): monkeypatch.setattr(runner.subprocess, "run", fake_run) monkeypatch.setattr(runner, "run_review", fail_review) + monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None) + monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: {"findings": [], "completeness": {}}) exit_code = runner.main( [ @@ -91,6 +93,8 @@ def fake_review(package: Path, repo_root: Path, python_executable: str) -> int: monkeypatch.setattr(runner.subprocess, "run", fake_git_diff) monkeypatch.setattr(runner, "run_review", fake_review) + monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None) + monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: []) exit_code = runner.main( [ @@ -134,6 +138,8 @@ def fail_review(*args, **kwargs): monkeypatch.setattr(runner.subprocess, "run", fake_git_diff) monkeypatch.setattr(runner, "run_review", fail_review) + monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None) + monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: []) exit_code = runner.main( [ @@ -184,6 +190,8 @@ def fake_review(package: Path, repo_root: Path, python_executable: str) -> int: monkeypatch.setattr(runner.subprocess, "run", fake_git_diff) monkeypatch.setattr(runner, "run_review", fake_review) + monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None) + monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: []) exit_code = runner.main( [ @@ -222,6 +230,8 @@ def fake_review(package: Path, repo_root: Path, python_executable: str) -> int: monkeypatch.setattr(runner.subprocess, "run", fake_git_diff) monkeypatch.setattr(runner, "run_review", fake_review) + monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None) + monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: {"findings": [], "completeness": {}}) exit_code = runner.main( [ @@ -258,6 +268,8 @@ def fake_review(package: Path, repo_root: Path, python_executable: str) -> int: monkeypatch.setattr(runner.subprocess, "run", fake_git_diff) monkeypatch.setattr(runner, "run_review", fake_review) + monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None) + monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: {"findings": [], "completeness": {}}) exit_code = runner.main( [ @@ -301,6 +313,8 @@ def fake_run(command, **kwargs): return _completed(command, returncode=1) monkeypatch.setattr(runner.subprocess, "run", fake_run) + monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None) + monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: []) exit_code = runner.main( [ @@ -339,6 +353,8 @@ def fake_review(package: Path, repo_root: Path, python_executable: str) -> int: return 0 monkeypatch.setattr(runner.subprocess, "run", fake_run) + monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None) + monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: []) monkeypatch.setattr(runner, "run_review", fake_review) exit_code = runner.main( @@ -375,6 +391,208 @@ def test_is_fully_removed_package_false_when_any_status_is_not_a_deletion(tmp_pa assert runner.is_fully_removed_package(package_rel, ["D", "M"], tmp_path) is False + +def _baseline_facts(*findings): + return {"findings": list(findings), "completeness": {}} + + +def _diff_stdout(): + sep = bytes([0]) + return b"M" + sep + b"skills/public/alpha/SKILL.md" + sep + + + +def test_main_fails_closed_when_head_review_crashes(tmp_path, monkeypatch, capsys): + _write_skill(tmp_path, "alpha") + diff_output = _diff_stdout() + + def fake_git_diff(command, **kwargs): + return _completed(command, stdout=diff_output) + + monkeypatch.setattr(runner.subprocess, "run", fake_git_diff) + monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None) + monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: None) + + exit_code = runner.main( + ["--base-ref", "base", "--head-ref", "head", "--repo-root", str(tmp_path)] + ) + output = capsys.readouterr().out + assert exit_code == 1 + assert "head review failed" in output + + +def test_main_fails_closed_when_head_review_returns_invalid_json(tmp_path, monkeypatch, capsys): + _write_skill(tmp_path, "alpha") + diff_output = _diff_stdout() + + def fake_git_diff(command, **kwargs): + return _completed(command, stdout=diff_output) + + monkeypatch.setattr(runner.subprocess, "run", fake_git_diff) + monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None) + monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: "not-a-dict") + + exit_code = runner.main( + ["--base-ref", "base", "--head-ref", "head", "--repo-root", str(tmp_path)] + ) + output = capsys.readouterr().out + assert exit_code == 1 + assert "head review failed" in output + + +def test_main_passes_when_baseline_finding_unchanged(tmp_path, monkeypatch, capsys): + _write_skill(tmp_path, "alpha") + diff_output = _diff_stdout() + finding = { + "path": "skills/public/alpha/SKILL.md", + "rule_id": "R001", + "line": 20, + "message": "pre-existing", + "severity": "error", + } + + def fake_git_diff(command, **kwargs): + return _completed(command, stdout=diff_output) + + monkeypatch.setattr(runner.subprocess, "run", fake_git_diff) + monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: tmp_path / "base-pkg") + monkeypatch.setattr( + runner, + "run_review_json", + lambda *a, **kw: _baseline_facts(finding) + if "base-pkg" in str(a[0]) + else _baseline_facts({**finding, "line": 30}), + ) + + exit_code = runner.main( + ["--base-ref", "base", "--head-ref", "head", "--repo-root", str(tmp_path)] + ) + output = capsys.readouterr().out + assert exit_code == 0 + assert "no new gating findings" in output + + +def test_main_fails_on_new_error_finding(tmp_path, monkeypatch, capsys): + _write_skill(tmp_path, "alpha") + diff_output = _diff_stdout() + new_error = { + "path": "skills/public/alpha/SKILL.md", + "rule_id": "R002", + "line": 5, + "message": "new error", + "severity": "error", + } + + def fake_git_diff(command, **kwargs): + return _completed(command, stdout=diff_output) + + monkeypatch.setattr(runner.subprocess, "run", fake_git_diff) + monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None) + monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: _baseline_facts(new_error)) + + exit_code = runner.main( + ["--base-ref", "base", "--head-ref", "head", "--repo-root", str(tmp_path)] + ) + output = capsys.readouterr().out + assert exit_code == 1 + assert "NEW error R002" in output + + +def test_main_passes_on_new_info_finding_preserving_severity_gate(tmp_path, monkeypatch, capsys): + _write_skill(tmp_path, "alpha") + diff_output = _diff_stdout() + new_info = { + "path": "skills/public/alpha/SKILL.md", + "rule_id": "R003", + "line": 5, + "message": "new info", + "severity": "info", + } + + def fake_git_diff(command, **kwargs): + return _completed(command, stdout=diff_output) + + monkeypatch.setattr(runner.subprocess, "run", fake_git_diff) + monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None) + monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: _baseline_facts(new_info)) + + exit_code = runner.main( + ["--base-ref", "base", "--head-ref", "head", "--repo-root", str(tmp_path)] + ) + output = capsys.readouterr().out + assert exit_code == 0 + assert "no new gating findings" in output + + +def test_main_fails_when_head_review_incomplete(tmp_path, monkeypatch, capsys): + _write_skill(tmp_path, "alpha") + diff_output = _diff_stdout() + + def fake_git_diff(command, **kwargs): + return _completed(command, stdout=diff_output) + + monkeypatch.setattr(runner.subprocess, "run", fake_git_diff) + monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: None) + monkeypatch.setattr( + runner, + "run_review_json", + lambda *a, **kw: {"findings": [], "completeness": {"not_assessed": ["SKILL.md"]}}, + ) + + exit_code = runner.main( + ["--base-ref", "base", "--head-ref", "head", "--repo-root", str(tmp_path)] + ) + output = capsys.readouterr().out + assert exit_code == 1 + assert "review incomplete" in output + + +def test_main_treats_all_as_new_when_baseline_review_fails(tmp_path, monkeypatch, capsys): + _write_skill(tmp_path, "alpha") + diff_output = _diff_stdout() + head_error = { + "path": "skills/public/alpha/SKILL.md", + "rule_id": "R004", + "line": 5, + "message": "head error", + "severity": "error", + } + + def fake_git_diff(command, **kwargs): + return _completed(command, stdout=diff_output) + + monkeypatch.setattr(runner.subprocess, "run", fake_git_diff) + monkeypatch.setattr(runner, "extract_package_at_ref", lambda *a, **kw: tmp_path / "base-pkg") + monkeypatch.setattr( + runner, + "run_review_json", + lambda *a, **kw: None + if "base-pkg" in str(a[0]) + else _baseline_facts(head_error), + ) + + exit_code = runner.main( + ["--base-ref", "base", "--head-ref", "head", "--repo-root", str(tmp_path)] + ) + output = capsys.readouterr().out + assert exit_code == 1 + assert "baseline review failed" in output + assert "NEW error R004" in output + + +def test_finding_key_excludes_line_number(): + base = {"path": "p", "rule_id": "R", "line": 20, "message": "m"} + head = {"path": "p", "rule_id": "R", "line": 30, "message": "m"} + assert runner.finding_key(base) == runner.finding_key(head) + + +def test_finding_gates_only_blocker_and_error(): + assert runner.finding_gates({"severity": "error"}) is True + assert runner.finding_gates({"severity": "blocker"}) is True + assert runner.finding_gates({"severity": "warning"}) is False + assert runner.finding_gates({"severity": "info"}) is False + assert runner.finding_gates({}) is False + def test_is_zero_sha_requires_full_sha_length() -> None: assert runner.is_zero_sha("0" * 40) is True assert runner.is_zero_sha("0" * 64) is True diff --git a/scripts/review_changed_public_skills.py b/scripts/review_changed_public_skills.py index f944c864d57..fde5c48dace 100644 --- a/scripts/review_changed_public_skills.py +++ b/scripts/review_changed_public_skills.py @@ -4,9 +4,12 @@ from __future__ import annotations import argparse +import json import os +import shutil import subprocess import sys +import tempfile from collections.abc import Sequence from dataclasses import dataclass from pathlib import Path, PurePosixPath @@ -43,10 +46,14 @@ def main(argv: Sequence[str] | None = None) -> int: if result.returncode != 0: fallback_args = build_force_push_fallback_diff_args(args) if fallback_args is None: - sys.stderr.write("[skill-review] Failed to collect changed public skill files.\n") + sys.stderr.write( + "[skill-review] Failed to collect changed public skill files.\n" + ) sys.stderr.write(result.stderr.decode("utf-8", errors="replace")) return result.returncode - sys.stderr.write("[skill-review] Primary push diff failed; falling back to empty-tree comparison.\n") + sys.stderr.write( + "[skill-review] Primary push diff failed; falling back to empty-tree comparison.\n" + ) sys.stderr.write(result.stderr.decode("utf-8", errors="replace")) diff_args = fallback_args print(f"[skill-review] Fallback diff: git diff {' '.join(diff_args)}") @@ -57,7 +64,9 @@ def main(argv: Sequence[str] | None = None) -> int: check=False, ) if result.returncode != 0: - sys.stderr.write("[skill-review] Failed to collect changed public skill files.\n") + sys.stderr.write( + "[skill-review] Failed to collect changed public skill files.\n" + ) sys.stderr.write(result.stderr.decode("utf-8", errors="replace")) return result.returncode @@ -68,13 +77,93 @@ def main(argv: Sequence[str] | None = None) -> int: return 0 print(f"[skill-review] Reviewing {len(packages)} changed public skill package(s).") + + base_ref = args.base_ref or args.before + use_baseline = bool(base_ref) + failed = False for package in packages: - if run_review(package, repo_root, args.python) != 0: - failed = True + package_rel = package.relative_to(repo_root).as_posix() + + if use_baseline: + base_pkg = extract_package_at_ref(package_rel, base_ref, repo_root) + if base_pkg is not None: + base_facts = run_review_json(base_pkg, repo_root, args.python) + if base_facts is not None: + base_keys = {finding_key(f) for f in base_facts.get("findings", [])} + else: + # Baseline review itself failed. Do not silently pass: + # treat every head finding as new (fail-closed). + base_keys = set() + print( + f"[skill-review] WARNING: baseline review failed at {base_ref}; " + f"treating all head findings as new" + ) + shutil.rmtree(base_pkg, ignore_errors=True) + # Clean up the temp root left behind by extract_package_at_ref. + tmp_root = base_pkg.parent + while tmp_root != tmp_root.parent and not any(tmp_root.iterdir()): + parent = tmp_root.parent + tmp_root.rmdir() + tmp_root = parent + print( + f"[skill-review] Baseline: {len(base_keys)} existing finding(s) at {base_ref}" + ) + else: + base_keys = set() + print( + f"[skill-review] Could not extract baseline at {base_ref}; treating all findings as new" + ) + + head_facts = run_review_json(package, repo_root, args.python) + if head_facts is None: + # Head review crashed or produced invalid output. This is NOT + # "no findings" — fail closed so the gate never silently passes. + print( + f"[skill-review] Failed: {package_rel} (head review failed / " + f"invalid output)" + ) + failed = True + continue + + new_findings = [ + f + for f in head_facts.get("findings", []) + if finding_key(f) not in base_keys + ] + gate_findings = [f for f in new_findings if finding_gates(f)] + incomplete = review_incomplete(head_facts) + + if gate_findings: + for f in gate_findings: + loc = f.get("path", "") + if f.get("line") is not None: + loc = f"{loc}:{f['line']}" + print( + f"[skill-review] NEW {f.get('severity')} {f.get('rule_id')} at {loc}: {f.get('message')}" + ) + print( + f"[skill-review] Failed: {package_rel} " + f"({len(gate_findings)} new gating finding(s))" + ) + failed = True + elif incomplete: + # Preserve the original --fail-on-incomplete semantics: a + # review that did not assess all content must not pass just + # because no new gating finding appeared. + print( + f"[skill-review] Failed: {package_rel} (review incomplete: " + f"{','.join(head_facts.get('completeness', {}).get('not_assessed') or []) or 'unknown'})" + ) + failed = True + else: + print(f"[skill-review] Passed: {package_rel} (no new gating findings)") + else: + if run_review(package, repo_root, args.python) != 0: + failed = True if failed: - print("[skill-review] One or more skill reviews failed.") + print("[skill-review] One or more skill reviews failed with new findings.") return 1 print("[skill-review] All changed public skill packages passed review.") @@ -82,7 +171,11 @@ def main(argv: Sequence[str] | None = None) -> int: def parse_args(argv: Sequence[str] | None) -> argparse.Namespace: - parser = argparse.ArgumentParser(description=("Review public skill packages whose SKILL.md changed in a PR or push diff.")) + parser = argparse.ArgumentParser( + description=( + "Review public skill packages whose SKILL.md changed in a PR or push diff." + ) + ) parser.add_argument( "--base-ref", "--base_ref", @@ -95,8 +188,12 @@ def parse_args(argv: Sequence[str] | None) -> argparse.Namespace: dest="head_ref", help="Head ref/SHA for PR-style base...head comparison.", ) - parser.add_argument("--before", help="Before SHA for push-style before/after comparison.") - parser.add_argument("--after", help="After SHA for push-style before/after comparison.") + parser.add_argument( + "--before", help="Before SHA for push-style before/after comparison." + ) + parser.add_argument( + "--after", help="After SHA for push-style before/after comparison." + ) parser.add_argument( "--repo-root", type=Path, @@ -113,7 +210,9 @@ def parse_args(argv: Sequence[str] | None) -> argparse.Namespace: has_pr_args = bool(args.base_ref or args.head_ref) has_push_args = bool(args.before or args.after) if has_pr_args == has_push_args: - parser.error("pass either --base-ref/--head-ref or --before/--after, but not both") + parser.error( + "pass either --base-ref/--head-ref or --before/--after, but not both" + ) if has_pr_args and not (args.base_ref and args.head_ref): parser.error("--base-ref and --head-ref must be provided together") if has_push_args and not (args.before and args.after): @@ -160,7 +259,9 @@ def parse_name_status(output: bytes) -> list[ChangedPath]: return changes -def select_skill_packages(changes: Sequence[ChangedPath], repo_root: Path) -> list[Path]: +def select_skill_packages( + changes: Sequence[ChangedPath], repo_root: Path +) -> list[Path]: package_statuses: dict[PurePosixPath, list[str]] = {} resolutions: list[tuple[ChangedPath, PurePosixPath]] = [] @@ -174,7 +275,9 @@ def select_skill_packages(changes: Sequence[ChangedPath], repo_root: Path) -> li package_rel = find_public_skill_package(change.path, repo_root) if package_rel is None: - print(f"[skill-review] Skipping path outside public skill package: {change.path}") + print( + f"[skill-review] Skipping path outside public skill package: {change.path}" + ) continue package_statuses.setdefault(package_rel, []).append(change.status) @@ -189,7 +292,9 @@ def select_skill_packages(changes: Sequence[ChangedPath], repo_root: Path) -> li continue seen.add(package_rel) - if is_fully_removed_package(package_rel, package_statuses[package_rel], repo_root): + if is_fully_removed_package( + package_rel, package_statuses[package_rel], repo_root + ): print(f"[skill-review] Skipping fully removed package: {package_rel}") continue @@ -199,7 +304,9 @@ def select_skill_packages(changes: Sequence[ChangedPath], repo_root: Path) -> li return packages -def is_fully_removed_package(package_rel: PurePosixPath, statuses: Sequence[str], repo_root: Path) -> bool: +def is_fully_removed_package( + package_rel: PurePosixPath, statuses: Sequence[str], repo_root: Path +) -> bool: """Whether every changed file that resolved to ``package_rel`` was a deletion and the package directory itself no longer exists on disk. @@ -215,7 +322,13 @@ def is_fully_removed_package(package_rel: PurePosixPath, statuses: Sequence[str] def is_public_skill_md(path: PurePosixPath) -> bool: parts = path.parts - return len(parts) >= 4 and parts[0] == "skills" and parts[1] == "public" and parts[-1] == "SKILL.md" and not _is_eval_fixture_skill_md(path) + return ( + len(parts) >= 4 + and parts[0] == "skills" + and parts[1] == "public" + and parts[-1] == "SKILL.md" + and not _is_eval_fixture_skill_md(path) + ) def is_public_skill_package_path(path: PurePosixPath) -> bool: @@ -223,14 +336,19 @@ def is_public_skill_package_path(path: PurePosixPath) -> bool: return len(parts) >= 3 and parts[0] == "skills" and parts[1] == "public" -def find_public_skill_package(path: PurePosixPath, repo_root: Path) -> PurePosixPath | None: +def find_public_skill_package( + path: PurePosixPath, repo_root: Path +) -> PurePosixPath | None: if not is_public_skill_package_path(path): return None current = path.parent if path.name else path while len(current.parts) >= 3: skill_md_rel = current / "SKILL.md" - if not _is_eval_fixture_skill_md(skill_md_rel) and (repo_root / skill_md_rel).is_file(): + if ( + not _is_eval_fixture_skill_md(skill_md_rel) + and (repo_root / skill_md_rel).is_file() + ): return current if len(current.parts) == 3: return current @@ -245,6 +363,98 @@ def _is_eval_fixture_skill_md(path: PurePosixPath) -> bool: return is_eval_fixture_skill_md(path) +def finding_key(finding: dict) -> tuple: + """Stable key for diffing findings across refs. + + Line numbers are excluded because unrelated insertions/deletions in the same + file shift line numbers for pre-existing findings. Path + rule_id + message + is a stable identity that survives typical code churn. + """ + return ( + finding.get("path", ""), + finding.get("rule_id", ""), + finding.get("message", ""), + ) + + +def run_review_json( + package: Path, repo_root: Path, python_executable: str, +) -> dict | None: + """Run review with JSON output, return the full facts dict. + + Returns ``None`` when the review itself failed (CLI crash, invalid JSON, + or empty output). Callers MUST treat ``None`` as REVIEW FAILED + (fail-closed) and never as "no findings". + """ + # The review CLI accepts an absolute skill directory path, so pass + # str(package) directly. base_pkg lives in a temp extraction dir (not + # under repo_root), so relative_to(repo_root) would raise ValueError for + # baseline reviews; cwd and env still come from the real repo root. + command = [ + python_executable, + "-m", + "deerflow.skills.review.cli", + str(package), + "--format", + "json", + ] + result = subprocess.run( + command, + cwd=repo_root, + env=review_env(repo_root), + capture_output=True, + check=False, + ) + if result.returncode != 0 or not result.stdout: + return None + try: + facts = json.loads(result.stdout.decode("utf-8", errors="replace")) + except (json.JSONDecodeError, ValueError): + return None + if not isinstance(facts, dict): + return None + return facts + + +def finding_gates(finding: dict) -> bool: + """Whether a finding blocks the gate under the original ``--fail-on error``. + + Only blocker/error findings gate; warning/info do not, matching the + pre-baseline CLI semantics so the baseline mode does not silently change + the severity policy. + """ + return str(finding.get("severity", "")).lower() in {"blocker", "error"} + + +def review_incomplete(facts: dict | None) -> bool: + """Whether the review left content not assessed (``--fail-on-incomplete``).""" + return bool((facts or {}).get("completeness", {}).get("not_assessed")) + + +def extract_package_at_ref(package_rel: str, ref: str, repo_root: Path) -> Path | None: + """Extract skill package at a git ref to a temp directory.""" + tmp_dir = Path(tempfile.mkdtemp(prefix="skill-review-")) + try: + result = subprocess.run( + ["git", "archive", ref, "--", package_rel], + cwd=repo_root, + capture_output=True, + check=False, + ) + if result.returncode != 0: + shutil.rmtree(tmp_dir, ignore_errors=True) + return None + subprocess.run(["tar", "-x"], cwd=tmp_dir, input=result.stdout, check=False) + extracted = tmp_dir / package_rel + if not extracted.is_dir(): + shutil.rmtree(tmp_dir, ignore_errors=True) + return None + return extracted + except OSError: + shutil.rmtree(tmp_dir, ignore_errors=True) + return None + + def run_review(package: Path, repo_root: Path, python_executable: str) -> int: package_rel = package.relative_to(repo_root).as_posix() command = [ @@ -289,7 +499,11 @@ def review_env(repo_root: Path) -> dict[str, str]: env = os.environ.copy() harness_path = repo_root / "backend" / "packages" / "harness" existing_pythonpath = env.get("PYTHONPATH") - env["PYTHONPATH"] = str(harness_path) if not existing_pythonpath else f"{harness_path}{os.pathsep}{existing_pythonpath}" + env["PYTHONPATH"] = ( + str(harness_path) + if not existing_pythonpath + else f"{harness_path}{os.pathsep}{existing_pythonpath}" + ) return env