diff --git a/.github/skill-review-waivers.v1.json b/.github/skill-review-waivers.v1.json new file mode 100644 index 000000000..8e1b4da41 --- /dev/null +++ b/.github/skill-review-waivers.v1.json @@ -0,0 +1,27 @@ +{ + "schema_version": "deerflow.skill-review-waivers.v1", + "waivers": [ + { + "package": "skills/public/skill-creator", + "source": "skillscan", + "rule_id": "python-subprocess", + "path": "scripts/improve_description.py", + "line": 35, + "evidence": "subprocess.run", + "file_sha256": "sha256:87d864570220b699fac52da309d2d6efdb060647bfebc74f768128e646accf80", + "reason": "Required Claude CLI invocation uses a fixed executable, an argv list, prompt input over stdin, and shell=False.", + "expires_on": "2027-02-28" + }, + { + "package": "skills/public/skill-creator", + "source": "skillscan", + "rule_id": "python-subprocess", + "path": "scripts/run_eval.py", + "line": 85, + "evidence": "subprocess.Popen", + "file_sha256": "sha256:43e3b8f80dbf69c343967ba77e268fae991d9fa3ed68b32a0ff02532cd48657f", + "reason": "Required Claude CLI invocation uses a fixed executable, an argv list, captured output streams, and shell=False.", + "expires_on": "2027-02-28" + } + ] +} diff --git a/.github/workflows/skill-review-ci.yml b/.github/workflows/skill-review-ci.yml index e46be4824..79570486d 100644 --- a/.github/workflows/skill-review-ci.yml +++ b/.github/workflows/skill-review-ci.yml @@ -8,6 +8,8 @@ on: - "backend/packages/harness/deerflow/skills/review/**" - "contracts/skill_review/**" - "scripts/review_changed_public_skills.py" + - "scripts/skill_review_waivers.py" + - ".github/skill-review-waivers.v1.json" - "backend/pyproject.toml" - "backend/uv.lock" - ".github/workflows/skill-review-ci.yml" @@ -18,6 +20,8 @@ on: - "backend/packages/harness/deerflow/skills/review/**" - "contracts/skill_review/**" - "scripts/review_changed_public_skills.py" + - "scripts/skill_review_waivers.py" + - ".github/skill-review-waivers.v1.json" - "backend/pyproject.toml" - "backend/uv.lock" - ".github/workflows/skill-review-ci.yml" diff --git a/AGENTS.md b/AGENTS.md index b42f1e50a..201497276 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -100,6 +100,14 @@ Skill quality review note: tag-neutralized; full raw payloads stay in tool artifacts. See [backend/AGENTS.md](backend/AGENTS.md) for the non-activation, SkillScan, and `skill-creator` ownership boundaries. +- CI waivers live in `.github/skill-review-waivers.v1.json` and are enforced by + `scripts/review_changed_public_skills.py`. Pull requests may validate waiver + edits from their head revision, but only the manifest from the trusted base + revision can suppress that run. Entries match one error finding exactly, + include the reviewed file's SHA-256 and an expiry date, remain visible in CI + output, and can never waive blocker findings. Adding a waiver and relying on + it therefore requires two steps: merge the reviewed waiver first, then update + the affected public skill in a later pull request. Scheduled-task note: - The scheduled-task MVP adds a workspace page at `/workspace/scheduled-tasks` plus a background scheduler service gated by `config.yaml -> scheduler.enabled`. diff --git a/backend/tests/test_review_changed_public_skills.py b/backend/tests/test_review_changed_public_skills.py index 5b2ef16f4..d9f89c39a 100644 --- a/backend/tests/test_review_changed_public_skills.py +++ b/backend/tests/test_review_changed_public_skills.py @@ -3,7 +3,9 @@ from __future__ import annotations import subprocess from pathlib import Path, PurePosixPath +import pytest import review_changed_public_skills as runner +from skill_review_waivers import EMPTY_MANIFEST, WaiverManifest def _completed(command: list[str], *, stdout: bytes = b"", returncode: int = 0) -> subprocess.CompletedProcess[bytes]: @@ -17,6 +19,11 @@ def _write_skill(repo_root: Path, package: str) -> Path: return skill_md +@pytest.fixture(autouse=True) +def _empty_waiver_manifests(monkeypatch) -> None: + monkeypatch.setattr(runner, "load_waiver_manifests", lambda args, repo_root: (EMPTY_MANIFEST, EMPTY_MANIFEST)) + + def test_main_skips_successfully_when_no_public_skill_changed(tmp_path: Path, monkeypatch, capsys) -> None: def fake_run(command, **kwargs): assert command == [ @@ -83,7 +90,7 @@ def test_main_reviews_changed_public_skill_and_skips_deleted_skill_md( assert command[:3] == ["git", "diff", "--name-status"] return _completed(command, stdout=diff_output) - def fake_review(package: Path, repo_root: Path, python_executable: str) -> int: + def fake_review(package: Path, repo_root: Path, python_executable: str, manifest: WaiverManifest) -> int: assert repo_root == tmp_path assert python_executable reviewed.append(package.relative_to(repo_root).as_posix()) @@ -178,7 +185,7 @@ def test_main_reviews_package_when_skill_md_deleted_but_sibling_file_remains( def fake_git_diff(command, **kwargs): return _completed(command, stdout=diff_output) - def fake_review(package: Path, repo_root: Path, python_executable: str) -> int: + def fake_review(package: Path, repo_root: Path, python_executable: str, manifest: WaiverManifest) -> int: reviewed.append(package.relative_to(repo_root).as_posix()) return 1 @@ -216,7 +223,7 @@ def test_main_reviews_package_when_only_support_file_changed( assert command[-1] == runner.PUBLIC_SKILL_PACKAGE_PATHSPEC return _completed(command, stdout=diff_output) - def fake_review(package: Path, repo_root: Path, python_executable: str) -> int: + def fake_review(package: Path, repo_root: Path, python_executable: str, manifest: WaiverManifest) -> int: reviewed.append(package.relative_to(repo_root).as_posix()) return 0 @@ -252,7 +259,7 @@ def test_main_maps_eval_fixture_changes_to_owner_package( def fake_git_diff(command, **kwargs): return _completed(command, stdout=diff_output) - def fake_review(package: Path, repo_root: Path, python_executable: str) -> int: + def fake_review(package: Path, repo_root: Path, python_executable: str, manifest: WaiverManifest) -> int: reviewed.append(package.relative_to(repo_root).as_posix()) return 0 @@ -290,15 +297,16 @@ def test_main_exits_nonzero_when_review_cli_reports_error(tmp_path: Path, monkey "deerflow.skills.review.cli", "skills/public/bad", "--format", - "text", + "json", "--fail-on", - "error", - "--fail-on-incomplete", + "never", ] assert kwargs["cwd"] == tmp_path assert "backend/packages/harness" in kwargs["env"]["PYTHONPATH"] + assert kwargs["capture_output"] is True + assert kwargs["text"] is True assert kwargs["check"] is False - return _completed(command, returncode=1) + return subprocess.CompletedProcess(command, 1, stdout="", stderr="analyzer error\n") monkeypatch.setattr(runner.subprocess, "run", fake_run) @@ -315,11 +323,12 @@ def test_main_exits_nonzero_when_review_cli_reports_error(tmp_path: Path, monkey ] ) - output = capsys.readouterr().out + captured = capsys.readouterr() assert exit_code == 1 assert [call[0] for call in calls] == ["git", "test-python"] - assert "Failed: skills/public/bad (exit 1)" in output - assert "One or more skill reviews failed." in output + assert "Failed: skills/public/bad" in captured.out + assert "One or more skill reviews failed." in captured.out + assert "Analyzer failed for skills/public/bad (exit 1)." in captured.err def test_main_falls_back_to_empty_tree_when_push_before_is_missing(tmp_path: Path, monkeypatch, capsys) -> None: @@ -334,7 +343,7 @@ def test_main_falls_back_to_empty_tree_when_push_before_is_missing(tmp_path: Pat return subprocess.CompletedProcess(command, 128, stdout=b"", stderr=b"fatal: bad object before") return _completed(command, stdout=diff_output) - def fake_review(package: Path, repo_root: Path, python_executable: str) -> int: + def fake_review(package: Path, repo_root: Path, python_executable: str, manifest: WaiverManifest) -> int: reviewed.append(package.relative_to(repo_root).as_posix()) return 0 diff --git a/backend/tests/test_skill_review_waivers.py b/backend/tests/test_skill_review_waivers.py new file mode 100644 index 000000000..abb7fc2bc --- /dev/null +++ b/backend/tests/test_skill_review_waivers.py @@ -0,0 +1,260 @@ +from __future__ import annotations + +import hashlib +import json +from datetime import date +from pathlib import Path +from types import SimpleNamespace + +import pytest +import review_changed_public_skills as runner +import skill_review_waivers as waiver_support +from jsonschema import Draft202012Validator, FormatChecker +from skill_review_waivers import ( + EMPTY_MANIFEST, + SCHEMA_VERSION, + SkillReviewWaiver, + WaiverManifest, + WaiverManifestError, + matching_waiver, + parse_manifest, + validate_manifest_against_facts, +) + +from deerflow.skills.review.analyzer import analyze_skill_package +from deerflow.skills.review.readers import LocalDirectoryReader + +REPO_ROOT = Path(__file__).resolve().parents[2] + + +def _write_target(repo_root: Path, content: bytes = b"safe subprocess invocation\n") -> tuple[Path, str]: + target = repo_root / "skills/public/demo/scripts/run.py" + target.parent.mkdir(parents=True, exist_ok=True) + target.write_bytes(content) + return target, f"sha256:{hashlib.sha256(content).hexdigest()}" + + +def _waiver(*, digest: str, expires_on: date = date(2027, 2, 28)) -> SkillReviewWaiver: + return SkillReviewWaiver( + package="skills/public/demo", + source="skillscan", + rule_id="python-subprocess", + path="scripts/run.py", + line=12, + evidence="subprocess.run", + file_sha256=digest, + reason="Fixed executable and argv invocation with shell disabled.", + expires_on=expires_on, + ) + + +def _finding(*, severity: str = "error", line: int = 12) -> dict[str, object]: + return { + "source": "skillscan", + "rule_id": "python-subprocess", + "path": "scripts/run.py", + "line": line, + "evidence": "subprocess.run", + "severity": severity, + "message": "Subprocess usage detected.", + } + + +def _payload(*, digest: str, path: str = "scripts/run.py", duplicate: bool = False) -> bytes: + entry = { + "package": "skills/public/demo", + "source": "skillscan", + "rule_id": "python-subprocess", + "path": path, + "line": 12, + "evidence": "subprocess.run", + "file_sha256": digest, + "reason": "Fixed executable and argv invocation with shell disabled.", + "expires_on": "2027-02-28", + } + return json.dumps({"schema_version": SCHEMA_VERSION, "waivers": [entry, entry] if duplicate else [entry]}).encode() + + +def test_committed_manifest_matches_schema_and_strict_parser() -> None: + manifest_path = REPO_ROOT / ".github/skill-review-waivers.v1.json" + schema = json.loads((REPO_ROOT / "contracts/skill_review/waiver_manifest.v1.schema.json").read_text(encoding="utf-8")) + payload = json.loads(manifest_path.read_text(encoding="utf-8")) + + Draft202012Validator(schema, format_checker=FormatChecker()).validate(payload) + parsed = parse_manifest(manifest_path.read_bytes(), source=str(manifest_path)) + + assert len(parsed.waivers) == 2 + + +def test_skill_creator_waivers_match_current_error_findings() -> None: + package = REPO_ROOT / "skills/public/skill-creator" + manifest = parse_manifest((REPO_ROOT / ".github/skill-review-waivers.v1.json").read_bytes(), source="committed manifest") + facts = analyze_skill_package(LocalDirectoryReader(package).read(), profile="deerflow") + + validation_errors = validate_manifest_against_facts( + manifest, + facts_by_package={"skills/public/skill-creator": facts}, + repo_root=REPO_ROOT, + today=date(2026, 8, 31), + ) + assert validation_errors == [] + + +@pytest.mark.parametrize("path", ["../run.py", "/tmp/run.py", "scripts\\run.py", "scripts/../run.py"]) +def test_parser_rejects_noncanonical_or_traversing_paths(tmp_path: Path, path: str) -> None: + _, digest = _write_target(tmp_path) + + with pytest.raises(WaiverManifestError, match="canonical relative POSIX path|backslashes"): + parse_manifest(_payload(digest=digest, path=path), source="test manifest") + + +def test_parser_rejects_duplicate_exact_waivers(tmp_path: Path) -> None: + _, digest = _write_target(tmp_path) + + with pytest.raises(WaiverManifestError, match="duplicates an earlier waiver"): + parse_manifest(_payload(digest=digest, duplicate=True), source="test manifest") + + +def test_missing_manifest_at_ref_means_no_waivers(tmp_path: Path, monkeypatch) -> None: + monkeypatch.setattr( + waiver_support.subprocess, + "run", + lambda *args, **kwargs: waiver_support.subprocess.CompletedProcess(args[0], 128, stdout=b"", stderr=b"missing"), + ) + + manifest = waiver_support.load_manifest_at_ref(tmp_path, "a" * 40, label="trusted base") + + assert manifest is EMPTY_MANIFEST + + +def test_matching_waiver_requires_exact_finding_and_current_file_hash(tmp_path: Path) -> None: + target, digest = _write_target(tmp_path) + manifest = WaiverManifest((_waiver(digest=digest),)) + + assert matching_waiver(_finding(), package="skills/public/demo", manifest=manifest, repo_root=tmp_path, today=date(2026, 8, 31)) is manifest.waivers[0] + assert matching_waiver(_finding(line=13), package="skills/public/demo", manifest=manifest, repo_root=tmp_path, today=date(2026, 8, 31)) is None + + target.write_bytes(b"changed\n") + assert matching_waiver(_finding(), package="skills/public/demo", manifest=manifest, repo_root=tmp_path, today=date(2026, 8, 31)) is None + + +def test_matching_waiver_rejects_symlinked_package_outside_repository(tmp_path: Path) -> None: + external_root = tmp_path.parent / f"{tmp_path.name}-external" + external_target, digest = _write_target(external_root) + public_root = tmp_path / "skills/public" + public_root.mkdir(parents=True) + (public_root / "demo").symlink_to(external_target.parents[1], target_is_directory=True) + + assert ( + matching_waiver( + _finding(), + package="skills/public/demo", + manifest=WaiverManifest((_waiver(digest=digest),)), + repo_root=tmp_path, + today=date(2026, 8, 31), + ) + is None + ) + + +def test_expired_waiver_is_rejected_and_does_not_match(tmp_path: Path) -> None: + _, digest = _write_target(tmp_path) + waiver = _waiver(digest=digest, expires_on=date(2026, 8, 30)) + manifest = WaiverManifest((waiver,)) + facts = {"findings": [_finding()]} + + errors = validate_manifest_against_facts( + manifest, + facts_by_package={waiver.package: facts}, + repo_root=tmp_path, + today=date(2026, 8, 31), + ) + + assert len(errors) == 1 + assert "expired on 2026-08-30" in errors[0] + assert matching_waiver(_finding(), package=waiver.package, manifest=manifest, repo_root=tmp_path, today=date(2026, 8, 31)) is None + + +def test_manifest_validation_refuses_blocker_waiver(tmp_path: Path) -> None: + _, digest = _write_target(tmp_path) + waiver = _waiver(digest=digest) + + errors = validate_manifest_against_facts( + WaiverManifest((waiver,)), + facts_by_package={waiver.package: {"findings": [_finding(severity="blocker")]}}, + repo_root=tmp_path, + today=date(2026, 8, 31), + ) + + assert "blockers can never be waived" in errors[0] + + +def test_pr_head_manifest_is_validated_but_cannot_self_apply(tmp_path: Path, monkeypatch) -> None: + _, digest = _write_target(tmp_path) + proposed = WaiverManifest((_waiver(digest=digest),)) + loaded: list[tuple[str, str]] = [] + + def fake_load(repo_root: Path, ref: str, *, label: str) -> WaiverManifest: + loaded.append((ref, label)) + return EMPTY_MANIFEST if label == "trusted base" else proposed + + monkeypatch.setattr(runner, "load_manifest_at_ref", fake_load) + args = SimpleNamespace(base_ref="base-sha", head_ref="head-sha", before=None, after=None) + + effective, head = runner.load_waiver_manifests(args, tmp_path) + + assert effective is EMPTY_MANIFEST + assert head is proposed + assert loaded == [("base-sha", "trusted base"), ("head-sha", "proposed head")] + + +def test_main_fails_closed_on_malformed_head_manifest(tmp_path: Path, monkeypatch, capsys) -> None: + def fail_load(args, repo_root): + raise WaiverManifestError("proposed head: invalid JSON") + + monkeypatch.setattr(runner, "load_waiver_manifests", fail_load) + monkeypatch.setattr(runner.subprocess, "run", lambda *args, **kwargs: pytest.fail("diff must not run")) + + exit_code = runner.main(["--base-ref", "base", "--head-ref", "head", "--repo-root", str(tmp_path)]) + + assert exit_code == 1 + assert "Invalid waiver manifest" in capsys.readouterr().err + + +def test_workflow_triggers_on_waiver_implementation_and_manifest() -> None: + workflow = (REPO_ROOT / ".github/workflows/skill-review-ci.yml").read_text(encoding="utf-8") + + assert workflow.count('"scripts/skill_review_waivers.py"') == 2 + assert workflow.count('".github/skill-review-waivers.v1.json"') == 2 + + +def test_run_review_keeps_waived_error_visible_and_passes(tmp_path: Path, monkeypatch, capsys) -> None: + package = tmp_path / "skills/public/demo" + _, digest = _write_target(tmp_path) + facts = { + "summary": {"blockers": 0, "errors": 1, "warnings": 0, "infos": 0}, + "completeness": {"not_assessed": []}, + "findings": [_finding()], + } + monkeypatch.setattr(runner, "collect_review_facts", lambda *args: facts) + + exit_code = runner.run_review(package, tmp_path, "python", WaiverManifest((_waiver(digest=digest),))) + + output = capsys.readouterr().out + assert exit_code == 0 + assert "- error python-subprocess" in output + assert "[WAIVED until 2027-02-28:" in output + assert "Passed: skills/public/demo (1 waived finding(s))" in output + + +def test_run_review_still_fails_for_unwaived_error(tmp_path: Path, monkeypatch) -> None: + package = tmp_path / "skills/public/demo" + _write_target(tmp_path) + facts = { + "summary": {"blockers": 0, "errors": 1, "warnings": 0, "infos": 0}, + "completeness": {"not_assessed": []}, + "findings": [_finding()], + } + monkeypatch.setattr(runner, "collect_review_facts", lambda *args: facts) + + assert runner.run_review(package, tmp_path, "python", EMPTY_MANIFEST) == 1 diff --git a/contracts/skill_review/waiver_manifest.v1.schema.json b/contracts/skill_review/waiver_manifest.v1.schema.json new file mode 100644 index 000000000..bd633bf54 --- /dev/null +++ b/contracts/skill_review/waiver_manifest.v1.schema.json @@ -0,0 +1,32 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://deerflow.dev/contracts/skill_review/waiver_manifest.v1.schema.json", + "title": "DeerFlow Skill Review Waiver Manifest v1", + "type": "object", + "required": ["schema_version", "waivers"], + "properties": { + "schema_version": { "const": "deerflow.skill-review-waivers.v1" }, + "waivers": { + "type": "array", + "maxItems": 256, + "items": { + "type": "object", + "required": ["package", "source", "rule_id", "path", "line", "evidence", "file_sha256", "reason", "expires_on"], + "properties": { + "package": { "type": "string", "pattern": "^skills/public/", "minLength": 15 }, + "source": { "type": "string", "minLength": 1 }, + "rule_id": { "type": "string", "minLength": 1 }, + "path": { "type": "string", "minLength": 1 }, + "line": { "type": "integer", "minimum": 1 }, + "evidence": { "type": "string", "minLength": 1 }, + "file_sha256": { "type": "string", "pattern": "^sha256:[0-9a-f]{64}$" }, + "reason": { "type": "string", "minLength": 20 }, + "expires_on": { "type": "string", "format": "date" }, + "approved_in": { "type": "string", "minLength": 1 } + }, + "additionalProperties": false + } + } + }, + "additionalProperties": false +} diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index e8a745bc3..3ef4969e9 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -15,6 +15,23 @@ likewise prefix the target with `bash`. This keeps documented `make` commands working when a source archive, `core.fileMode=false`, or a non-POSIX filesystem does not preserve executable bits. +## Public Skill Review Waivers + +`review_changed_public_skills.py` keeps the analyzer strict and applies narrow +CI-only exceptions from `.github/skill-review-waivers.v1.json`. The manifest is +versioned by `contracts/skill_review/waiver_manifest.v1.schema.json`; each entry +must identify one current error by package, source, rule, path, line, and +evidence, and pin the complete source file with SHA-256 plus an expiry date. +Blockers are never waivable, and waived errors are still printed with their +original severity and justification. + +For pull requests, only the base revision's manifest is effective. The head +manifest is parsed and checked against the current analyzer output, but cannot +self-authorize a finding in the same pull request. Push comparisons use the +same before/after trust boundary. A waiver-only change can therefore land +without weakening its own check, then become effective for later changes after +it is part of the trusted base. + ## Backend Static Analysis Commands The root `detect-thread-boundaries` target statically inventories execution diff --git a/scripts/review_changed_public_skills.py b/scripts/review_changed_public_skills.py index f944c864d..338f3a290 100644 --- a/scripts/review_changed_public_skills.py +++ b/scripts/review_changed_public_skills.py @@ -4,12 +4,23 @@ 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" @@ -31,6 +42,17 @@ def main(argv: Sequence[str] | None = None) -> int: 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)}") @@ -70,7 +92,7 @@ def main(argv: Sequence[str] | None = None) -> int: 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) != 0: + if run_review(package, repo_root, args.python, effective_manifest) != 0: failed = True if failed: @@ -81,6 +103,39 @@ def main(argv: Sequence[str] | None = None) -> int: 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( @@ -245,7 +300,7 @@ def _is_eval_fixture_skill_md(path: PurePosixPath) -> bool: return is_eval_fixture_skill_md(path) -def run_review(package: Path, repo_root: Path, python_executable: str) -> int: +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, @@ -253,36 +308,72 @@ def run_review(package: Path, repo_root: Path, python_executable: str) -> int: "deerflow.skills.review.cli", package_rel, "--format", - "text", + "json", "--fail-on", - "error", - "--fail-on-incomplete", + "never", ] - log_command = [ - "python", - "-m", - "deerflow.skills.review.cli", - package_rel, - "--format", - "text", - "--fail-on", - "error", - "--fail-on-incomplete", - ] - - print(f"[skill-review] Reviewing package: {package_rel}") - print(f"[skill-review] $ {' '.join(log_command)}") result = subprocess.run( command, cwd=repo_root, env=review_env(repo_root), + capture_output=True, + text=True, check=False, ) - if result.returncode == 0: - print(f"[skill-review] Passed: {package_rel}") - else: - print(f"[skill-review] Failed: {package_rel} (exit {result.returncode})") - return result.returncode + 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 "" + 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]: diff --git a/scripts/skill_review_waivers.py b/scripts/skill_review_waivers.py new file mode 100644 index 000000000..4fc8d2916 --- /dev/null +++ b/scripts/skill_review_waivers.py @@ -0,0 +1,272 @@ +"""Trusted, exact-match waivers for the public-skill CI review gate.""" + +from __future__ import annotations + +import hashlib +import json +import re +import subprocess +from dataclasses import dataclass +from datetime import date +from pathlib import Path, PurePosixPath +from typing import Any + +SCHEMA_VERSION = "deerflow.skill-review-waivers.v1" +MANIFEST_PATH = PurePosixPath(".github/skill-review-waivers.v1.json") +_SHA256_RE = re.compile(r"sha256:[0-9a-f]{64}\Z") +_SAFE_REF_RE = re.compile(r"[A-Za-z0-9][A-Za-z0-9._/-]*\Z") +_REQUIRED_ENTRY_FIELDS = { + "package", + "source", + "rule_id", + "path", + "line", + "evidence", + "file_sha256", + "reason", + "expires_on", +} +_OPTIONAL_ENTRY_FIELDS = {"approved_in"} + + +class WaiverManifestError(ValueError): + """Raised when a waiver manifest is missing required trust properties.""" + + +@dataclass(frozen=True) +class SkillReviewWaiver: + package: str + source: str + rule_id: str + path: str + line: int + evidence: str + file_sha256: str + reason: str + expires_on: date + approved_in: str | None = None + + @property + def finding_key(self) -> tuple[str, str, str, int, str]: + return (self.source, self.rule_id, self.path, self.line, self.evidence) + + +@dataclass(frozen=True) +class WaiverManifest: + waivers: tuple[SkillReviewWaiver, ...] = () + + +EMPTY_MANIFEST = WaiverManifest() + + +def parse_manifest(payload: bytes | str, *, source: str) -> WaiverManifest: + """Parse the versioned waiver manifest with strict, fail-closed validation.""" + try: + data = json.loads(payload) + except (json.JSONDecodeError, UnicodeDecodeError, TypeError) as exc: + raise WaiverManifestError(f"{source}: invalid JSON: {exc}") from exc + + if not isinstance(data, dict): + raise WaiverManifestError(f"{source}: manifest root must be an object") + unknown_root = set(data) - {"schema_version", "waivers"} + if unknown_root: + raise WaiverManifestError(f"{source}: unknown root field(s): {', '.join(sorted(unknown_root))}") + if data.get("schema_version") != SCHEMA_VERSION: + raise WaiverManifestError(f"{source}: schema_version must be {SCHEMA_VERSION!r}") + raw_waivers = data.get("waivers") + if not isinstance(raw_waivers, list): + raise WaiverManifestError(f"{source}: waivers must be an array") + if len(raw_waivers) > 256: + raise WaiverManifestError(f"{source}: waivers must contain at most 256 entries") + + waivers: list[SkillReviewWaiver] = [] + identities: set[tuple[str, str, str, int, str, str]] = set() + for index, raw in enumerate(raw_waivers): + entry_source = f"{source}: waivers[{index}]" + if not isinstance(raw, dict): + raise WaiverManifestError(f"{entry_source}: entry must be an object") + fields = set(raw) + missing = _REQUIRED_ENTRY_FIELDS - fields + unknown = fields - _REQUIRED_ENTRY_FIELDS - _OPTIONAL_ENTRY_FIELDS + if missing: + raise WaiverManifestError(f"{entry_source}: missing field(s): {', '.join(sorted(missing))}") + if unknown: + raise WaiverManifestError(f"{entry_source}: unknown field(s): {', '.join(sorted(unknown))}") + + package = _canonical_relative_path(raw["package"], field=f"{entry_source}.package") + if not package.startswith("skills/public/"): + raise WaiverManifestError(f"{entry_source}.package: must be below skills/public/") + path = _canonical_relative_path(raw["path"], field=f"{entry_source}.path") + source_name = _nonempty_string(raw["source"], field=f"{entry_source}.source") + rule_id = _nonempty_string(raw["rule_id"], field=f"{entry_source}.rule_id") + evidence = _nonempty_string(raw["evidence"], field=f"{entry_source}.evidence") + file_sha256 = _nonempty_string(raw["file_sha256"], field=f"{entry_source}.file_sha256") + if not _SHA256_RE.fullmatch(file_sha256): + raise WaiverManifestError(f"{entry_source}.file_sha256: must be sha256 followed by 64 lowercase hex characters") + reason = _nonempty_string(raw["reason"], field=f"{entry_source}.reason") + if len(reason) < 20: + raise WaiverManifestError(f"{entry_source}.reason: must contain at least 20 characters") + line = raw["line"] + if isinstance(line, bool) or not isinstance(line, int) or line < 1: + raise WaiverManifestError(f"{entry_source}.line: must be a positive integer") + expires_on = _parse_date(raw["expires_on"], field=f"{entry_source}.expires_on") + approved_in = raw.get("approved_in") + if approved_in is not None: + approved_in = _nonempty_string(approved_in, field=f"{entry_source}.approved_in") + + waiver = SkillReviewWaiver( + package=package, + source=source_name, + rule_id=rule_id, + path=path, + line=line, + evidence=evidence, + file_sha256=file_sha256, + reason=reason, + expires_on=expires_on, + approved_in=approved_in, + ) + identity = (*waiver.finding_key, waiver.package) + if identity in identities: + raise WaiverManifestError(f"{entry_source}: duplicates an earlier waiver") + identities.add(identity) + waivers.append(waiver) + + return WaiverManifest(tuple(waivers)) + + +def load_manifest_at_ref(repo_root: Path, ref: str, *, label: str) -> WaiverManifest: + """Read a manifest from a Git ref; absence means no waivers at that ref.""" + if not _SAFE_REF_RE.fullmatch(ref) or ref.startswith("-") or ".." in ref or ":" in ref: + raise WaiverManifestError(f"{label}: unsafe Git ref {ref!r}") + object_name = f"{ref}:{MANIFEST_PATH.as_posix()}" + result = subprocess.run( + ["git", "show", object_name], + cwd=repo_root, + capture_output=True, + check=False, + ) + if result.returncode != 0: + return EMPTY_MANIFEST + return parse_manifest(result.stdout, source=f"{label} manifest at {ref}") + + +def file_sha256(repo_root: Path, waiver: SkillReviewWaiver) -> str | None: + resolved_repo_root = repo_root.resolve() + package_candidate = resolved_repo_root / waiver.package + if package_candidate.is_symlink() or not package_candidate.is_dir(): + return None + package_root = package_candidate.resolve() + try: + package_root.relative_to(resolved_repo_root / "skills" / "public") + except ValueError: + return None + candidate = package_root / waiver.path + if candidate.is_symlink() or not candidate.is_file(): + return None + resolved = candidate.resolve() + try: + resolved.relative_to(package_root) + except ValueError: + return None + digest = hashlib.sha256(resolved.read_bytes()).hexdigest() + return f"sha256:{digest}" + + +def finding_key(finding: dict[str, Any]) -> tuple[str, str, str, int, str] | None: + source = finding.get("source") + rule_id = finding.get("rule_id") + path = finding.get("path") + line = finding.get("line") + evidence = finding.get("evidence") + if not isinstance(source, str) or not isinstance(rule_id, str) or not isinstance(path, str) or isinstance(line, bool) or not isinstance(line, int) or not isinstance(evidence, str): + return None + return (source, rule_id, path, line, evidence) + + +def matching_waiver( + finding: dict[str, Any], + *, + package: str, + manifest: WaiverManifest, + repo_root: Path, + today: date | None = None, +) -> SkillReviewWaiver | None: + """Return an active waiver only for an exact error finding and file digest.""" + if finding.get("severity") != "error": + return None + key = finding_key(finding) + if key is None: + return None + current_date = today or date.today() + for waiver in manifest.waivers: + if waiver.package != package or waiver.finding_key != key: + continue + if waiver.expires_on < current_date: + continue + if file_sha256(repo_root, waiver) != waiver.file_sha256: + continue + return waiver + return None + + +def validate_manifest_against_facts( + manifest: WaiverManifest, + *, + facts_by_package: dict[str, dict[str, Any]], + repo_root: Path, + today: date | None = None, +) -> list[str]: + """Reject expired, stale, hash-mismatched, or non-error waiver entries.""" + errors: list[str] = [] + current_date = today or date.today() + for waiver in manifest.waivers: + description = f"{waiver.package}/{waiver.path}:{waiver.line} ({waiver.source}/{waiver.rule_id})" + if waiver.expires_on < current_date: + errors.append(f"{description}: waiver expired on {waiver.expires_on.isoformat()}") + continue + facts = facts_by_package.get(waiver.package) + if facts is None: + errors.append(f"{description}: package review facts are unavailable") + continue + matches = [finding for finding in facts.get("findings", []) if isinstance(finding, dict) and finding_key(finding) == waiver.finding_key] + if not matches: + errors.append(f"{description}: no exact current finding matches this waiver") + continue + if len(matches) != 1: + errors.append(f"{description}: expected one exact current finding, found {len(matches)}") + continue + if any(finding.get("severity") != "error" for finding in matches): + errors.append(f"{description}: waivers may target error findings only; blockers can never be waived") + continue + actual_hash = file_sha256(repo_root, waiver) + if actual_hash != waiver.file_sha256: + errors.append(f"{description}: file digest changed (expected {waiver.file_sha256}, found {actual_hash or 'unavailable'})") + return errors + + +def _canonical_relative_path(value: object, *, field: str) -> str: + text = _nonempty_string(value, field=field) + if "\\" in text: + raise WaiverManifestError(f"{field}: backslashes are not allowed") + path = PurePosixPath(text) + if path.is_absolute() or any(part in {"", ".", ".."} for part in path.parts) or path.as_posix() != text: + raise WaiverManifestError(f"{field}: must be a canonical relative POSIX path without traversal") + return text + + +def _nonempty_string(value: object, *, field: str) -> str: + if not isinstance(value, str) or not value.strip() or value != value.strip(): + raise WaiverManifestError(f"{field}: must be a non-empty string without surrounding whitespace") + return value + + +def _parse_date(value: object, *, field: str) -> date: + text = _nonempty_string(value, field=field) + try: + parsed = date.fromisoformat(text) + except ValueError as exc: + raise WaiverManifestError(f"{field}: must be an ISO 8601 calendar date") from exc + if parsed.isoformat() != text: + raise WaiverManifestError(f"{field}: must use YYYY-MM-DD format") + return parsed