mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-14 16:08:41 +00:00
1af79c7bcf
* fix(ci):resolve the skill_review errors * fix(ci): split skill creator fixes from waiver rollout
393 lines
15 KiB
Python
393 lines
15 KiB
Python
#!/usr/bin/env python3
|
|
"""Run deterministic skill review for changed public skills."""
|
|
|
|
from __future__ import annotations
|
|
|
|
import argparse
|
|
import json
|
|
import os
|
|
import subprocess
|
|
import sys
|
|
from collections.abc import Sequence
|
|
from dataclasses import dataclass
|
|
from pathlib import Path, PurePosixPath
|
|
from typing import Any
|
|
|
|
from skill_review_waivers import (
|
|
EMPTY_MANIFEST,
|
|
WaiverManifest,
|
|
WaiverManifestError,
|
|
load_manifest_at_ref,
|
|
matching_waiver,
|
|
validate_manifest_against_facts,
|
|
)
|
|
|
|
REPO_ROOT = Path(__file__).resolve().parents[1]
|
|
HARNESS_PATH = REPO_ROOT / "backend" / "packages" / "harness"
|
|
if HARNESS_PATH.is_dir():
|
|
sys.path.insert(0, str(HARNESS_PATH))
|
|
|
|
PUBLIC_SKILL_PACKAGE_PATHSPEC = ":(glob)skills/public/**"
|
|
EMPTY_TREE_SHA = "4b825dc642cb6eb9a060e54bf8d69288fbee4904"
|
|
|
|
|
|
@dataclass(frozen=True)
|
|
class ChangedPath:
|
|
status: str
|
|
path: PurePosixPath
|
|
|
|
|
|
def main(argv: Sequence[str] | None = None) -> int:
|
|
args = parse_args(argv)
|
|
repo_root = args.repo_root.resolve()
|
|
diff_args = build_diff_args(args)
|
|
|
|
try:
|
|
effective_manifest, proposed_manifest = load_waiver_manifests(args, repo_root)
|
|
except WaiverManifestError as exc:
|
|
sys.stderr.write(f"[skill-review] Invalid waiver manifest: {exc}\n")
|
|
return 1
|
|
|
|
print(f"[skill-review] Trusted waivers: {len(effective_manifest.waivers)}")
|
|
print(f"[skill-review] Proposed waivers validated but not trusted in this run: {len(proposed_manifest.waivers)}")
|
|
if validate_proposed_manifest(proposed_manifest, repo_root, args.python) != 0:
|
|
return 1
|
|
|
|
print(f"[skill-review] Repository: {repo_root}")
|
|
print(f"[skill-review] Diff: git diff {' '.join(diff_args)}")
|
|
|
|
result = subprocess.run(
|
|
["git", "diff", *diff_args, "--", PUBLIC_SKILL_PACKAGE_PATHSPEC],
|
|
cwd=repo_root,
|
|
capture_output=True,
|
|
check=False,
|
|
)
|
|
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(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(result.stderr.decode("utf-8", errors="replace"))
|
|
diff_args = fallback_args
|
|
print(f"[skill-review] Fallback diff: git diff {' '.join(diff_args)}")
|
|
result = subprocess.run(
|
|
["git", "diff", *diff_args, "--", PUBLIC_SKILL_PACKAGE_PATHSPEC],
|
|
cwd=repo_root,
|
|
capture_output=True,
|
|
check=False,
|
|
)
|
|
if result.returncode != 0:
|
|
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
|
|
|
|
changes = parse_name_status(result.stdout)
|
|
packages = select_skill_packages(changes, repo_root)
|
|
if not packages:
|
|
print("[skill-review] No changed public skill package files; skipping review.")
|
|
return 0
|
|
|
|
print(f"[skill-review] Reviewing {len(packages)} changed public skill package(s).")
|
|
failed = False
|
|
for package in packages:
|
|
if run_review(package, repo_root, args.python, effective_manifest) != 0:
|
|
failed = True
|
|
|
|
if failed:
|
|
print("[skill-review] One or more skill reviews failed.")
|
|
return 1
|
|
|
|
print("[skill-review] All changed public skill packages passed review.")
|
|
return 0
|
|
|
|
|
|
def load_waiver_manifests(args: argparse.Namespace, repo_root: Path) -> tuple[WaiverManifest, WaiverManifest]:
|
|
"""Load effective waivers only from the trusted side of the comparison."""
|
|
if args.base_ref and args.head_ref:
|
|
effective = load_manifest_at_ref(repo_root, str(args.base_ref), label="trusted base")
|
|
proposed = load_manifest_at_ref(repo_root, str(args.head_ref), label="proposed head")
|
|
return effective, proposed
|
|
|
|
before = str(args.before)
|
|
effective = EMPTY_MANIFEST if is_zero_sha(before) else load_manifest_at_ref(repo_root, before, label="trusted before")
|
|
proposed = load_manifest_at_ref(repo_root, str(args.after), label="proposed after")
|
|
return effective, proposed
|
|
|
|
|
|
def validate_proposed_manifest(manifest: WaiverManifest, repo_root: Path, python_executable: str) -> int:
|
|
"""Verify every proposed entry still identifies one current error finding."""
|
|
if not manifest.waivers:
|
|
return 0
|
|
|
|
facts_by_package: dict[str, dict[str, Any]] = {}
|
|
for package_rel in sorted({waiver.package for waiver in manifest.waivers}):
|
|
package = repo_root / package_rel
|
|
print(f"[skill-review] Validating proposed waivers for: {package_rel}")
|
|
facts = collect_review_facts(package, repo_root, python_executable)
|
|
if facts is None:
|
|
return 1
|
|
facts_by_package[package_rel] = facts
|
|
|
|
errors = validate_manifest_against_facts(manifest, facts_by_package=facts_by_package, repo_root=repo_root)
|
|
for error in errors:
|
|
sys.stderr.write(f"[skill-review] Invalid proposed waiver: {error}\n")
|
|
return 1 if errors else 0
|
|
|
|
|
|
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.add_argument(
|
|
"--base-ref",
|
|
"--base_ref",
|
|
dest="base_ref",
|
|
help="Base ref/SHA for PR-style base...head comparison.",
|
|
)
|
|
parser.add_argument(
|
|
"--head-ref",
|
|
"--head_ref",
|
|
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(
|
|
"--repo-root",
|
|
type=Path,
|
|
default=Path(__file__).resolve().parents[1],
|
|
help="Repository root. Defaults to this script's parent repository.",
|
|
)
|
|
parser.add_argument(
|
|
"--python",
|
|
default=sys.executable,
|
|
help="Python executable used to invoke python -m deerflow.skills.review.cli.",
|
|
)
|
|
args = parser.parse_args(argv)
|
|
|
|
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")
|
|
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):
|
|
parser.error("--before and --after must be provided together")
|
|
|
|
return args
|
|
|
|
|
|
def build_diff_args(args: argparse.Namespace) -> list[str]:
|
|
if args.base_ref and args.head_ref:
|
|
return ["--name-status", "-z", f"{args.base_ref}...{args.head_ref}"]
|
|
|
|
before = str(args.before)
|
|
after = str(args.after)
|
|
if is_zero_sha(before):
|
|
return ["--name-status", "-z", EMPTY_TREE_SHA, after]
|
|
return ["--name-status", "-z", before, after]
|
|
|
|
|
|
def build_force_push_fallback_diff_args(args: argparse.Namespace) -> list[str] | None:
|
|
if not args.before or not args.after or is_zero_sha(str(args.before)):
|
|
return None
|
|
return ["--name-status", "-z", EMPTY_TREE_SHA, str(args.after)]
|
|
|
|
|
|
def parse_name_status(output: bytes) -> list[ChangedPath]:
|
|
parts = [part for part in output.split(b"\0") if part]
|
|
changes: list[ChangedPath] = []
|
|
index = 0
|
|
while index < len(parts):
|
|
status = parts[index].decode("utf-8", errors="surrogateescape")
|
|
index += 1
|
|
if not status:
|
|
continue
|
|
|
|
path_index = index + 1 if status[0] in {"C", "R"} else index
|
|
if path_index >= len(parts):
|
|
raise ValueError(f"Malformed git diff --name-status output near {status!r}")
|
|
|
|
path = parts[path_index].decode("utf-8", errors="surrogateescape")
|
|
changes.append(ChangedPath(status=status, path=PurePosixPath(path)))
|
|
index = path_index + 1
|
|
|
|
return changes
|
|
|
|
|
|
def select_skill_packages(changes: Sequence[ChangedPath], repo_root: Path) -> list[Path]:
|
|
package_statuses: dict[PurePosixPath, list[str]] = {}
|
|
resolutions: list[tuple[ChangedPath, PurePosixPath]] = []
|
|
|
|
for change in changes:
|
|
if not is_public_skill_package_path(change.path):
|
|
continue
|
|
|
|
if change.status.startswith("D") and is_public_skill_md(change.path):
|
|
print(f"[skill-review] Skipping deleted SKILL.md: {change.path}")
|
|
continue
|
|
|
|
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}")
|
|
continue
|
|
|
|
package_statuses.setdefault(package_rel, []).append(change.status)
|
|
resolutions.append((change, package_rel))
|
|
|
|
packages: list[Path] = []
|
|
seen: set[PurePosixPath] = set()
|
|
|
|
for _, package_rel in resolutions:
|
|
if package_rel in seen:
|
|
print(f"[skill-review] Already queued package: {package_rel}")
|
|
continue
|
|
seen.add(package_rel)
|
|
|
|
if is_fully_removed_package(package_rel, package_statuses[package_rel], repo_root):
|
|
print(f"[skill-review] Skipping fully removed package: {package_rel}")
|
|
continue
|
|
|
|
packages.append(repo_root / package_rel)
|
|
print(f"[skill-review] Queued package: {package_rel}")
|
|
|
|
return packages
|
|
|
|
|
|
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.
|
|
|
|
This identifies a whole public skill package being intentionally deleted (all of its
|
|
files removed, not just SKILL.md), as distinct from a package left in a broken/partial
|
|
state (e.g. SKILL.md deleted while other package files remain on disk) — the latter
|
|
must still be reviewed and flagged.
|
|
"""
|
|
if not all(status.startswith("D") for status in statuses):
|
|
return False
|
|
return not (repo_root / package_rel).is_dir()
|
|
|
|
|
|
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)
|
|
|
|
|
|
def is_public_skill_package_path(path: PurePosixPath) -> bool:
|
|
parts = path.parts
|
|
return len(parts) >= 3 and parts[0] == "skills" and parts[1] == "public"
|
|
|
|
|
|
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():
|
|
return current
|
|
if len(current.parts) == 3:
|
|
return current
|
|
current = current.parent
|
|
|
|
return None
|
|
|
|
|
|
def _is_eval_fixture_skill_md(path: PurePosixPath) -> bool:
|
|
from deerflow.skills.package_paths import is_eval_fixture_skill_md
|
|
|
|
return is_eval_fixture_skill_md(path)
|
|
|
|
|
|
def collect_review_facts(package: Path, repo_root: Path, python_executable: str) -> dict[str, Any] | None:
|
|
package_rel = package.relative_to(repo_root).as_posix()
|
|
command = [
|
|
python_executable,
|
|
"-m",
|
|
"deerflow.skills.review.cli",
|
|
package_rel,
|
|
"--format",
|
|
"json",
|
|
"--fail-on",
|
|
"never",
|
|
]
|
|
result = subprocess.run(
|
|
command,
|
|
cwd=repo_root,
|
|
env=review_env(repo_root),
|
|
capture_output=True,
|
|
text=True,
|
|
check=False,
|
|
)
|
|
if result.returncode != 0:
|
|
sys.stderr.write(f"[skill-review] Analyzer failed for {package_rel} (exit {result.returncode}).\n")
|
|
sys.stderr.write(result.stderr)
|
|
return None
|
|
try:
|
|
facts = json.loads(result.stdout)
|
|
except (json.JSONDecodeError, TypeError) as exc:
|
|
sys.stderr.write(f"[skill-review] Analyzer returned invalid JSON for {package_rel}: {exc}\n")
|
|
return None
|
|
if not isinstance(facts, dict):
|
|
sys.stderr.write(f"[skill-review] Analyzer returned a non-object payload for {package_rel}.\n")
|
|
return None
|
|
return facts
|
|
|
|
|
|
def run_review(package: Path, repo_root: Path, python_executable: str, manifest: WaiverManifest = EMPTY_MANIFEST) -> int:
|
|
package_rel = package.relative_to(repo_root).as_posix()
|
|
print(f"[skill-review] Reviewing package: {package_rel}")
|
|
print(f"[skill-review] $ python -m deerflow.skills.review.cli {package_rel} --format json --fail-on never")
|
|
facts = collect_review_facts(package, repo_root, python_executable)
|
|
if facts is None:
|
|
print(f"[skill-review] Failed: {package_rel}")
|
|
return 1
|
|
|
|
summary = facts.get("summary", {})
|
|
completeness = facts.get("completeness", {})
|
|
print(f"[skill-review] Summary: {summary.get('blockers')} blocker(s), {summary.get('errors')} error(s), {summary.get('warnings')} warning(s), {summary.get('infos')} info(s)")
|
|
not_assessed = completeness.get("not_assessed") or []
|
|
failed = bool(not_assessed)
|
|
if not_assessed:
|
|
print(f"[skill-review] Incomplete review: {', '.join(str(item) for item in not_assessed)}")
|
|
|
|
waived_count = 0
|
|
for finding in facts.get("findings", []):
|
|
if not isinstance(finding, dict):
|
|
failed = True
|
|
continue
|
|
location = finding.get("path") or "<package>"
|
|
if finding.get("line") is not None:
|
|
location = f"{location}:{finding['line']}"
|
|
waiver = matching_waiver(finding, package=package_rel, manifest=manifest, repo_root=repo_root)
|
|
waiver_suffix = ""
|
|
if waiver is not None:
|
|
waived_count += 1
|
|
waiver_suffix = f" [WAIVED until {waiver.expires_on.isoformat()}: {waiver.reason}]"
|
|
elif finding.get("severity") in {"blocker", "error"}:
|
|
failed = True
|
|
print(f"- {finding.get('severity')} {finding.get('rule_id')} at {location}: {finding.get('message')}{waiver_suffix}")
|
|
|
|
if failed:
|
|
print(f"[skill-review] Failed: {package_rel}")
|
|
return 1
|
|
print(f"[skill-review] Passed: {package_rel} ({waived_count} waived finding(s))")
|
|
return 0
|
|
|
|
|
|
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}"
|
|
return env
|
|
|
|
|
|
def is_zero_sha(value: str) -> bool:
|
|
return len(value) in {40, 64} and set(value) == {"0"}
|
|
|
|
|
|
if __name__ == "__main__":
|
|
sys.exit(main())
|