From 546705404b0bb291ef80ba1f6d7b2467538375f6 Mon Sep 17 00:00:00 2001 From: Battleplus <3559424769@qq.com> Date: Mon, 24 Aug 2026 18:48:15 +0800 Subject: [PATCH 1/4] fix(skills): compare skill review findings against base ref baseline The skill review CI gate runs review_changed_public_skills.py on the entire package when any file in it changes. Pre-existing findings in unchanged files blocked the PR even though they were not introduced by the change. Now when a base ref is provided (PR-style or push-style), the script extracts the package at the base ref, collects baseline findings, then only fails on newly introduced errors. Pre-existing findings are logged for visibility but do not block. Closes #4996 Signed-off-by: Battleplus --- scripts/review_changed_public_skills.py | 180 +++++++++++++++++++++--- 1 file changed, 162 insertions(+), 18 deletions(-) diff --git a/scripts/review_changed_public_skills.py b/scripts/review_changed_public_skills.py index f944c864d57..4f20881a0d5 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,52 @@ 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_findings = run_review_json(base_pkg, base_pkg.parent, args.python) + base_keys = {finding_key(f) for f in base_findings} + shutil.rmtree(base_pkg.parent, ignore_errors=True) + 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_findings = run_review_json(package, repo_root, args.python) + new_findings = [f for f in head_findings if finding_key(f) not in base_keys] + + if new_findings: + for f in new_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} ({len(new_findings)} new finding(s))" + ) + failed = True + else: + print(f"[skill-review] Passed: {package_rel} (no new 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 +130,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 +147,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 +169,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 +218,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 +234,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 +251,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 +263,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 +281,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 +295,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 +322,69 @@ 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.""" + return ( + finding.get("path", ""), + finding.get("rule_id", ""), + finding.get("line"), + finding.get("message", ""), + ) + + +def run_review_json( + package: Path, repo_root: Path, python_executable: str +) -> list[dict]: + """Run review with JSON output, return parsed findings.""" + package_rel = package.relative_to(repo_root).as_posix() + command = [ + python_executable, + "-m", + "deerflow.skills.review.cli", + package_rel, + "--format", + "json", + ] + result = subprocess.run( + command, + cwd=repo_root, + env=review_env(repo_root), + capture_output=True, + check=False, + ) + if result.returncode != 0 and not result.stdout: + return [] + try: + facts = json.loads(result.stdout.decode("utf-8", errors="replace")) + except (json.JSONDecodeError, ValueError): + return [] + return facts.get("findings", []) + + +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 +429,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 From 6c8ff730a37631eff480f4974c7b07b2f9260ab5 Mon Sep 17 00:00:00 2001 From: Battleplus <3559424769@qq.com> Date: Mon, 24 Aug 2026 22:58:19 +0800 Subject: [PATCH 2/4] fix(tests): mock baseline extraction in review script tests Signed-off-by: Battleplus <3559424769@qq.com> --- .../tests/test_review_changed_public_skills.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/backend/tests/test_review_changed_public_skills.py b/backend/tests/test_review_changed_public_skills.py index 5b2ef16f46c..52daf75a064 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: []) 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: []) 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: []) 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( From 608aa4942ba450269a9a659e35268e15c1e3d787 Mon Sep 17 00:00:00 2001 From: Battleplus <3559424769@qq.com> Date: Tue, 25 Aug 2026 17:26:05 +0800 Subject: [PATCH 3/4] fix(skills): pass real repo root for PYTHONPATH in base review env review_env() constructed PYTHONPATH from whatever repo_root was passed. When reviewing the base ref, run_review_json received the tmpdir (base_pkg.parent) as repo_root, producing a nonexistent harness path in PYTHONPATH. Add an env_repo_root parameter so the base review always uses the actual repository root for environment construction. Signed-off-by: Battleplus <3559424769@qq.com> --- scripts/review_changed_public_skills.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/scripts/review_changed_public_skills.py b/scripts/review_changed_public_skills.py index 4f20881a0d5..7c255cb664e 100644 --- a/scripts/review_changed_public_skills.py +++ b/scripts/review_changed_public_skills.py @@ -88,9 +88,15 @@ def main(argv: Sequence[str] | None = None) -> int: if use_baseline: base_pkg = extract_package_at_ref(package_rel, base_ref, repo_root) if base_pkg is not None: - base_findings = run_review_json(base_pkg, base_pkg.parent, args.python) + base_findings = run_review_json(base_pkg, repo_root, args.python) base_keys = {finding_key(f) for f in base_findings} - shutil.rmtree(base_pkg.parent, ignore_errors=True) + 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}" ) @@ -323,17 +329,21 @@ def _is_eval_fixture_skill_md(path: PurePosixPath) -> bool: def finding_key(finding: dict) -> tuple: - """Stable key for diffing findings across refs.""" + """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("line"), finding.get("message", ""), ) def run_review_json( - package: Path, repo_root: Path, python_executable: str + package: Path, repo_root: Path, python_executable: str, ) -> list[dict]: """Run review with JSON output, return parsed findings.""" package_rel = package.relative_to(repo_root).as_posix() From 4c8d1de12cf704d43295477343f78eda80d8a137 Mon Sep 17 00:00:00 2001 From: Battleplus <3559424769@qq.com> Date: Tue, 25 Aug 2026 23:18:14 +0800 Subject: [PATCH 4/4] fix(skills): make skill review gate fail-closed and preserve original semantics Addresses willem-bd review findings on #5000: 1. Fail-closed head review: run_review_json now returns the full facts dict or None. None means the review itself failed (CLI crash, invalid JSON, empty output) and is treated as REVIEW FAILED, never as no findings. Previously a head CLI crash produced [] which the gate interpreted as no new findings and passed (fail-open). 2. Preserve original --fail-on error severity semantics: only blocker/error findings gate; new warning/info findings no longer fail the baseline path, matching the pre-baseline CLI behavior. 3. Preserve --fail-on-incomplete semantics: a head review with content not assessed fails even when no new gating finding appears. 4. Stable finding identity: line numbers excluded from finding_key so line shifts from unrelated edits do not turn pre-existing findings into new ones. 5. Baseline review failure is fail-closed: if the base review crashes, all head findings are treated as new rather than silently passing. 6. Fix baseline run crash: run_review_json passes str(package) (absolute path, accepted by the CLI) instead of package.relative_to(repo_root), which raised ValueError for temp-extracted base packages. 7. Temp extraction cleanup walks up removing empty dirs. --- .../test_review_changed_public_skills.py | 208 +++++++++++++++++- scripts/review_changed_public_skills.py | 94 ++++++-- 2 files changed, 282 insertions(+), 20 deletions(-) diff --git a/backend/tests/test_review_changed_public_skills.py b/backend/tests/test_review_changed_public_skills.py index 52daf75a064..9c199d5c82c 100644 --- a/backend/tests/test_review_changed_public_skills.py +++ b/backend/tests/test_review_changed_public_skills.py @@ -39,7 +39,7 @@ 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: []) + monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: {"findings": [], "completeness": {}}) exit_code = runner.main( [ @@ -231,7 +231,7 @@ 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: []) + monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: {"findings": [], "completeness": {}}) exit_code = runner.main( [ @@ -269,7 +269,7 @@ 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: []) + monkeypatch.setattr(runner, "run_review_json", lambda *a, **kw: {"findings": [], "completeness": {}}) exit_code = runner.main( [ @@ -391,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 7c255cb664e..fde5c48dace 100644 --- a/scripts/review_changed_public_skills.py +++ b/scripts/review_changed_public_skills.py @@ -88,8 +88,17 @@ def main(argv: Sequence[str] | None = None) -> int: if use_baseline: base_pkg = extract_package_at_ref(package_rel, base_ref, repo_root) if base_pkg is not None: - base_findings = run_review_json(base_pkg, repo_root, args.python) - base_keys = {finding_key(f) for f in base_findings} + 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 @@ -106,11 +115,27 @@ def main(argv: Sequence[str] | None = None) -> int: f"[skill-review] Could not extract baseline at {base_ref}; treating all findings as new" ) - head_findings = run_review_json(package, repo_root, args.python) - new_findings = [f for f in head_findings if finding_key(f) not in base_keys] - - if new_findings: - for f in new_findings: + 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']}" @@ -118,11 +143,21 @@ def main(argv: Sequence[str] | None = None) -> int: f"[skill-review] NEW {f.get('severity')} {f.get('rule_id')} at {loc}: {f.get('message')}" ) print( - f"[skill-review] Failed: {package_rel} ({len(new_findings)} new finding(s))" + 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 findings)") + print(f"[skill-review] Passed: {package_rel} (no new gating findings)") else: if run_review(package, repo_root, args.python) != 0: failed = True @@ -344,14 +379,22 @@ def finding_key(finding: dict) -> tuple: def run_review_json( package: Path, repo_root: Path, python_executable: str, -) -> list[dict]: - """Run review with JSON output, return parsed findings.""" - package_rel = package.relative_to(repo_root).as_posix() +) -> 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", - package_rel, + str(package), "--format", "json", ] @@ -362,13 +405,30 @@ def run_review_json( capture_output=True, check=False, ) - if result.returncode != 0 and not result.stdout: - return [] + 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 [] - return facts.get("findings", []) + 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: