diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d202b1d..2b2b3fd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -27,6 +27,16 @@ jobs: - uses: actions/checkout@v4 with: fetch-depth: 0 - - uses: ./ + - id: sheriff + uses: ./ with: base: HEAD^ + comment: "false" + mode: advisory + - name: Verify Action outputs + env: + RISK: ${{ steps.sheriff.outputs.risk }} + POLICY_PASSED: ${{ steps.sheriff.outputs.policy-passed }} + run: | + test -n "$RISK" + test "$POLICY_PASSED" = "true" -o "$POLICY_PASSED" = "false" diff --git a/.pr-sheriff.json b/.pr-sheriff.json index 918361f..f73c5e8 100644 --- a/.pr-sheriff.json +++ b/.pr-sheriff.json @@ -4,5 +4,6 @@ "require_tests_after_lines": 120, "test_patterns": ["tests/**", "test/**", "**/*_test.*", "**/*.test.*", "**/*.spec.*"], "sensitive_patterns": [".github/workflows/**", "**/auth/**", "**/security/**", "**/migrations/**", "Dockerfile", "**/Dockerfile", "package-lock.json", "poetry.lock"], - "ignore_patterns": ["**/*.md", "docs/**"] + "ignore_patterns": ["**/*.md", "docs/**"], + "path_rules": [] } diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d56f2e..ef8683a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ All notable changes to PR Sheriff are documented here. +## 0.4.0 - 2026-06-06 + +- Add path-specific thresholds and test requirements. +- Show an explainable risk score breakdown in every report. +- Add advisory mode for gradual, non-blocking adoption. +- Validate numeric thresholds and nested path rule configuration. +- Expose `policy-passed` as a GitHub Action output. + ## 0.3.1 - 2026-06-06 - Publish PR Sheriff to the GitHub Actions Marketplace. diff --git a/README.md b/README.md index a00aec8..5643c90 100644 --- a/README.md +++ b/README.md @@ -43,6 +43,12 @@ Policy check passed. The command exits with `1` when policy violations exist, making it suitable for CI and pre-push hooks. Use `--json` for integrations. +Start with `--advisory` to report violations without blocking contributors: + +```bash +pr-sheriff check --base origin/main --advisory +``` + ## What it checks - Maximum changed lines and files @@ -75,7 +81,7 @@ jobs: - uses: actions/checkout@v4 with: fetch-depth: 0 - - uses: Hayal08/pr-sheriff@v0.3.1 + - uses: Hayal08/pr-sheriff@v0.4.0 with: base: origin/${{ github.base_ref }} ``` @@ -92,9 +98,26 @@ comment, and fails when policy is violated. | `head` | `HEAD` | Head git ref for the three-dot diff | | `config` | `.pr-sheriff.json` | Path to repository policy | | `comment` | `true` | Create or update a report comment on pull requests | +| `mode` | `enforce` | Use `advisory` to report without failing the check | The Action exposes `risk`, `score`, `changed-files`, `changed-lines`, and -`tests-changed` outputs for later workflow steps. +`tests-changed` outputs for later workflow steps. The `policy-passed` output is +`false` when violations exist, including in advisory mode. + +### Gradual rollout + +Use advisory mode to learn what the policy would flag before making it a +required check: + +```yaml +- uses: Hayal08/pr-sheriff@v0.4.0 + with: + base: origin/${{ github.base_ref }} + mode: advisory +``` + +Advisory mode keeps the Job Summary, PR comment, outputs, and annotations, but +turns policy errors into warnings and returns a successful exit code. ## Configuration @@ -107,13 +130,48 @@ Run `pr-sheriff init` or add `.pr-sheriff.json` manually: "require_tests_after_lines": 120, "test_patterns": ["tests/**", "**/*_test.*", "**/*.test.*"], "sensitive_patterns": [".github/workflows/**", "**/auth/**", "**/migrations/**"], - "ignore_patterns": ["**/*.md", "docs/**"] + "ignore_patterns": ["**/*.md", "docs/**"], + "path_rules": [] } ``` Unknown configuration keys are rejected so typos cannot silently weaken a policy. +### Path-specific rules + +Different parts of a repository can have stricter review policies. Each matched +rule is shown separately in the report: + +```json +{ + "path_rules": [ + { + "name": "database migrations", + "patterns": ["**/migrations/**"], + "max_changed_lines": 100, + "require_tests_after_lines": 1 + }, + { + "name": "frontend", + "patterns": ["web/**"], + "max_changed_files": 15, + "require_tests_after_lines": 80 + } + ] +} +``` + +Path rules support `max_changed_lines`, `max_changed_files`, and +`require_tests_after_lines`. Global limits still apply. + +### Explainable risk score + +Human-readable, JSON, Job Summary, and PR comment reports now show how changed +lines, changed files, and sensitive paths contribute to the risk score. JSON +consumers can use `score_breakdown` and `path_rule_results` for custom +dashboards or workflow decisions. + ## Try it safely Open a pull request that changes more than `require_tests_after_lines` without diff --git a/ROADMAP.md b/ROADMAP.md index 8b98dc7..e8313e0 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -13,16 +13,19 @@ review. The roadmap is intentionally small and driven by maintainer feedback. ## Next: better policy checks - Require changelog entries for user-visible changes. -- Support path-specific thresholds and policies. - Detect dependency and public API changes. + +Recently completed: + +- Support path-specific thresholds and policies. - Explain how each part of the risk score was calculated. +- Add a non-blocking advisory mode for gradual adoption. ## Later: larger repositories - Add first-class monorepo support. - Report risk separately for affected packages. - Support reusable organization-wide policy presets. -- Explore a non-blocking advisory mode for gradual adoption. ## Not planned diff --git a/action.yml b/action.yml index 239df44..c2a097e 100644 --- a/action.yml +++ b/action.yml @@ -23,6 +23,10 @@ inputs: description: Create or update a report comment on pull requests required: false default: "true" + mode: + description: Policy mode, either enforce or advisory + required: false + default: enforce outputs: risk: @@ -40,6 +44,9 @@ outputs: tests-changed: description: Whether the pull request changes tests value: ${{ steps.check.outputs.tests-changed }} + policy-passed: + description: Whether the pull request satisfies every configured policy + value: ${{ steps.check.outputs.policy-passed }} runs: using: composite @@ -51,12 +58,20 @@ runs: PR_SHERIFF_HEAD: ${{ inputs.head }} PR_SHERIFF_CONFIG: ${{ inputs.config }} PR_SHERIFF_COMMENT: ${{ inputs.comment }} + PR_SHERIFF_MODE: ${{ inputs.mode }} GITHUB_TOKEN: ${{ github.token }} run: | comment_args=() if [[ "$PR_SHERIFF_COMMENT" == "true" ]]; then comment_args+=(--github-comment) fi + mode_args=() + if [[ "$PR_SHERIFF_MODE" == "advisory" ]]; then + mode_args+=(--advisory) + elif [[ "$PR_SHERIFF_MODE" != "enforce" ]]; then + echo "::error::mode must be either enforce or advisory" + exit 2 + fi PYTHONPATH="$GITHUB_ACTION_PATH/src" python -m pr_sheriff check \ --base "$PR_SHERIFF_BASE" \ --head "$PR_SHERIFF_HEAD" \ @@ -64,4 +79,5 @@ runs: --github-summary "$GITHUB_STEP_SUMMARY" \ --github-output "$GITHUB_OUTPUT" \ --github-annotations \ - "${comment_args[@]}" + "${comment_args[@]}" \ + "${mode_args[@]}" diff --git a/pyproject.toml b/pyproject.toml index 4cfaee8..0e7551c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "pr-sheriff" -version = "0.3.1" +version = "0.4.0" description = "Deterministic pull request risk checks for busy maintainers" readme = "README.md" requires-python = ">=3.10" diff --git a/src/pr_sheriff/__init__.py b/src/pr_sheriff/__init__.py index f787c78..dc4a4fe 100644 --- a/src/pr_sheriff/__init__.py +++ b/src/pr_sheriff/__init__.py @@ -1,3 +1,3 @@ """PR Sheriff: deterministic pull request risk checks.""" -__version__ = "0.3.1" +__version__ = "0.4.0" diff --git a/src/pr_sheriff/cli.py b/src/pr_sheriff/cli.py index 2e6dc36..c7ca2ed 100644 --- a/src/pr_sheriff/cli.py +++ b/src/pr_sheriff/cli.py @@ -20,6 +20,9 @@ def build_parser() -> argparse.ArgumentParser: check.add_argument("--head", default="HEAD", help="head git ref") check.add_argument("--config", default=".pr-sheriff.json", type=Path) check.add_argument("--json", action="store_true", dest="as_json") + check.add_argument( + "--advisory", action="store_true", help="report violations without failing" + ) check.add_argument("--github-summary", type=Path, help=argparse.SUPPRESS) check.add_argument("--github-output", type=Path, help=argparse.SUPPRESS) check.add_argument("--github-annotations", action="store_true", help=argparse.SUPPRESS) @@ -32,6 +35,13 @@ def build_parser() -> argparse.ArgumentParser: def print_report(report) -> None: print(f"PR risk: {report.risk.upper()} ({report.score}/100)") print(f"Changed: {report.changed_files} files, {report.changed_lines} lines") + if report.score_breakdown: + print( + "Score: " + f"lines +{report.score_breakdown['changed_lines']}, " + f"files +{report.score_breakdown['changed_files']}, " + f"sensitive +{report.score_breakdown['sensitive_files']}" + ) if report.sensitive_files: print("Sensitive files:") for path in report.sensitive_files: @@ -44,8 +54,8 @@ def print_report(report) -> None: print("Policy check passed.") -def markdown_report(report) -> str: - status = "Failed" if report.violations else "Passed" +def markdown_report(report, advisory: bool = False) -> str: + status = "Advisory" if advisory and report.violations else "Failed" if report.violations else "Passed" tests = "yes" if report.tests_changed else "no" lines = [ "## PR Sheriff report", @@ -56,6 +66,31 @@ def markdown_report(report) -> str: "| ---: | ---: | :---: |", f"| {report.changed_files} | {report.changed_lines} | {tests} |", ] + if report.score_breakdown: + breakdown = report.score_breakdown + lines.extend( + [ + "", + "
", + "Risk score breakdown", + "", + "| Changed lines | Changed files | Sensitive files | Cap adjustment | Total |", + "| ---: | ---: | ---: | ---: | ---: |", + f"| +{breakdown['changed_lines']} | +{breakdown['changed_files']} | " + f"+{breakdown['sensitive_files']} | {breakdown['cap_adjustment']} | " + f"**{breakdown['total']}** |", + "", + "
", + ] + ) + if report.path_rule_results: + lines.extend(["", "### Matched path rules"]) + for result in report.path_rule_results: + status_text = "violated" if result["violations"] else "passed" + lines.append( + f"- **{result['name']}**: {result['changed_files']} files, " + f"{result['changed_lines']} lines ({status_text})" + ) if report.sensitive_files: lines.extend(["", "### Sensitive files"]) lines.extend(f"- `{path}`" for path in report.sensitive_files) @@ -76,17 +111,19 @@ def write_github_output(path: Path, report) -> None: "changed-files": report.changed_files, "changed-lines": report.changed_lines, "tests-changed": str(report.tests_changed).lower(), + "policy-passed": str(not report.violations).lower(), } with path.open("a", encoding="utf-8") as output: for key, value in values.items(): output.write(f"{key}={value}\n") -def print_github_annotations(report) -> None: +def print_github_annotations(report, advisory: bool = False) -> None: for path in report.sensitive_files: print(f"::warning file={github_escape(path)}::Sensitive file changed") for violation in report.violations: - print(f"::error::{github_escape(violation)}") + level = "warning" if advisory else "error" + print(f"::{level}::{github_escape(violation)}") def main(argv: list[str] | None = None) -> int: @@ -111,11 +148,11 @@ def main(argv: list[str] | None = None) -> int: print_report(report) if args.github_summary: with args.github_summary.open("a", encoding="utf-8") as summary: - summary.write(markdown_report(report)) + summary.write(markdown_report(report, args.advisory)) if args.github_output: write_github_output(args.github_output, report) if args.github_annotations: - print_github_annotations(report) + print_github_annotations(report, args.advisory) if args.github_comment: try: event_path = os.environ.get("GITHUB_EVENT_PATH") @@ -126,7 +163,11 @@ def main(argv: list[str] | None = None) -> int: number = pull_request_number(Path(event_path)) if number: result = upsert_pull_request_comment( - markdown_report(report), token, repository, number, api_url + markdown_report(report, args.advisory), + token, + repository, + number, + api_url, ) print(f"PR comment {result}.") else: @@ -135,7 +176,7 @@ def main(argv: list[str] | None = None) -> int: print("PR comment skipped: GitHub environment is incomplete.") except (OSError, RuntimeError, ValueError, json.JSONDecodeError) as exc: print(f"::warning::PR comment skipped: {github_escape(str(exc))}") - return 1 if report.violations else 0 + return 1 if report.violations and not args.advisory else 0 if __name__ == "__main__": diff --git a/src/pr_sheriff/core.py b/src/pr_sheriff/core.py index 3820ea9..bf08442 100644 --- a/src/pr_sheriff/core.py +++ b/src/pr_sheriff/core.py @@ -1,6 +1,6 @@ from __future__ import annotations -from dataclasses import asdict, dataclass +from dataclasses import asdict, dataclass, field from fnmatch import fnmatch import json from pathlib import Path @@ -24,6 +24,15 @@ "poetry.lock", ], "ignore_patterns": ["**/*.md", "docs/**"], + "path_rules": [], +} + +PATH_RULE_KEYS = { + "name", + "patterns", + "max_changed_lines", + "max_changed_files", + "require_tests_after_lines", } @@ -47,6 +56,8 @@ class Report: tests_changed: bool sensitive_files: list[str] violations: list[str] + score_breakdown: dict[str, int] = field(default_factory=dict) + path_rule_results: list[dict] = field(default_factory=list) def to_dict(self) -> dict: return asdict(self) @@ -89,37 +100,123 @@ def git_changes(base: str, head: str = "HEAD") -> list[FileChange]: def load_config(path: Path) -> dict: - config = DEFAULT_CONFIG.copy() + config = {**DEFAULT_CONFIG, "path_rules": []} if path.exists(): loaded = json.loads(path.read_text(encoding="utf-8")) + if not isinstance(loaded, dict): + raise ValueError("configuration must be a JSON object") unknown = sorted(set(loaded) - set(DEFAULT_CONFIG)) if unknown: raise ValueError(f"unknown configuration keys: {', '.join(unknown)}") config.update(loaded) + validate_config(config) return config +def validate_config(config: dict) -> None: + numeric_keys = ("max_changed_lines", "max_changed_files", "require_tests_after_lines") + for key in numeric_keys: + value = config[key] + if isinstance(value, bool) or not isinstance(value, int) or value < 0: + raise ValueError(f"{key} must be a non-negative integer") + for key in ("test_patterns", "sensitive_patterns", "ignore_patterns"): + if not isinstance(config[key], list) or not all( + isinstance(item, str) for item in config[key] + ): + raise ValueError(f"{key} must be a list of strings") + if not isinstance(config["path_rules"], list): + raise ValueError("path_rules must be a list") + names = set() + for index, rule in enumerate(config["path_rules"]): + prefix = f"path_rules[{index}]" + if not isinstance(rule, dict): + raise ValueError(f"{prefix} must be an object") + unknown = sorted(set(rule) - PATH_RULE_KEYS) + if unknown: + raise ValueError(f"{prefix} has unknown keys: {', '.join(unknown)}") + name = rule.get("name") + patterns = rule.get("patterns") + if not isinstance(name, str) or not name.strip(): + raise ValueError(f"{prefix}.name must be a non-empty string") + if name in names: + raise ValueError(f"path rule name must be unique: {name}") + names.add(name) + if not isinstance(patterns, list) or not patterns or not all( + isinstance(item, str) for item in patterns + ): + raise ValueError(f"{prefix}.patterns must be a non-empty list of strings") + for key in numeric_keys: + if key in rule: + value = rule[key] + if isinstance(value, bool) or not isinstance(value, int) or value < 0: + raise ValueError(f"{prefix}.{key} must be a non-negative integer") + + +def check_limits( + changes: list[FileChange], + tests_changed: bool, + limits: dict, + prefix: str = "", +) -> list[str]: + changed_lines = sum(change.lines for change in changes) + violations = [] + if "max_changed_lines" in limits and changed_lines > limits["max_changed_lines"]: + violations.append( + f"{prefix}changed lines {changed_lines} exceed limit {limits['max_changed_lines']}" + ) + if "max_changed_files" in limits and len(changes) > limits["max_changed_files"]: + violations.append( + f"{prefix}changed files {len(changes)} exceed limit {limits['max_changed_files']}" + ) + if ( + "require_tests_after_lines" in limits + and changed_lines >= limits["require_tests_after_lines"] + and not tests_changed + ): + violations.append( + f"{prefix}tests required for changes of " + f"{limits['require_tests_after_lines']}+ lines" + ) + return violations + + def analyze(changes: list[FileChange], config: dict) -> Report: relevant = [c for c in changes if not matches(c.path, config["ignore_patterns"])] changed_lines = sum(c.lines for c in relevant) tests_changed = any(matches(c.path, config["test_patterns"]) for c in changes) sensitive = [c.path for c in changes if matches(c.path, config["sensitive_patterns"])] - violations = [] - if changed_lines > config["max_changed_lines"]: - violations.append( - f"changed lines {changed_lines} exceed limit {config['max_changed_lines']}" - ) - if len(relevant) > config["max_changed_files"]: - violations.append( - f"changed files {len(relevant)} exceed limit {config['max_changed_files']}" + violations = check_limits(relevant, tests_changed, config) + path_rule_results = [] + for rule in config["path_rules"]: + rule_changes = [change for change in relevant if matches(change.path, rule["patterns"])] + if not rule_changes: + continue + rule_violations = check_limits( + rule_changes, tests_changed, rule, prefix=f"[{rule['name']}] " ) - if changed_lines >= config["require_tests_after_lines"] and not tests_changed: - violations.append( - f"tests required for changes of {config['require_tests_after_lines']}+ lines" + violations.extend(rule_violations) + path_rule_results.append( + { + "name": rule["name"], + "changed_files": len(rule_changes), + "changed_lines": sum(change.lines for change in rule_changes), + "violations": rule_violations, + } ) - score = min(100, changed_lines // 10 + len(relevant) * 2 + len(sensitive) * 25) + line_points = changed_lines // 10 + file_points = len(relevant) * 2 + sensitive_points = len(sensitive) * 25 + uncapped_score = line_points + file_points + sensitive_points + score = min(100, uncapped_score) + score_breakdown = { + "changed_lines": line_points, + "changed_files": file_points, + "sensitive_files": sensitive_points, + "cap_adjustment": score - uncapped_score, + "total": score, + } risk = "high" if score >= 60 else "medium" if score >= 25 else "low" return Report( risk=risk, @@ -129,4 +226,6 @@ def analyze(changes: list[FileChange], config: dict) -> Report: tests_changed=tests_changed, sensitive_files=sensitive, violations=violations, + score_breakdown=score_breakdown, + path_rule_results=path_rule_results, ) diff --git a/tests/test_cli.py b/tests/test_cli.py index 6f43ad3..fbabdff 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -4,6 +4,7 @@ from pathlib import Path import tempfile import unittest +from unittest.mock import patch from pr_sheriff.cli import ( github_escape, @@ -44,6 +45,7 @@ def test_github_outputs_are_appended(self): output = path.read_text() self.assertIn("risk=low\n", output) self.assertIn("tests-changed=true\n", output) + self.assertIn("policy-passed=true\n", output) def test_annotations_escape_workflow_commands(self): report = Report("high", 80, 4, 900, False, ["a%b.py"], ["bad\nchange"]) @@ -53,6 +55,35 @@ def test_annotations_escape_workflow_commands(self): self.assertIn("file=a%25b.py", output.getvalue()) self.assertIn("bad%0Achange", output.getvalue()) + def test_advisory_mode_returns_success_for_violations(self): + report = Report("high", 80, 4, 900, False, [], ["too large"]) + with patch("pr_sheriff.cli.git_changes", return_value=[]), patch( + "pr_sheriff.cli.analyze", return_value=report + ): + self.assertEqual(main(["check", "--base", "HEAD", "--advisory"]), 0) + + def test_markdown_lists_matched_path_rules_and_breakdown(self): + report = Report( + "medium", + 30, + 2, + 30, + True, + [], + [], + { + "changed_lines": 3, + "changed_files": 2, + "sensitive_files": 25, + "cap_adjustment": 0, + "total": 30, + }, + [{"name": "api", "changed_files": 2, "changed_lines": 30, "violations": []}], + ) + markdown = markdown_report(report) + self.assertIn("Risk score breakdown", markdown) + self.assertIn("**api**: 2 files, 30 lines (passed)", markdown) + if __name__ == "__main__": unittest.main() diff --git a/tests/test_core.py b/tests/test_core.py index e4cca99..aab4a90 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1,7 +1,17 @@ +import json +from pathlib import Path +import tempfile import unittest from unittest.mock import patch -from pr_sheriff.core import DEFAULT_CONFIG, FileChange, analyze, git_changes, parse_numstat +from pr_sheriff.core import ( + DEFAULT_CONFIG, + FileChange, + analyze, + git_changes, + load_config, + parse_numstat, +) class ParseNumstatTests(unittest.TestCase): @@ -42,6 +52,8 @@ def test_sensitive_file_increases_risk(self): ) self.assertEqual(report.risk, "medium") self.assertEqual(report.sensitive_files, [".github/workflows/release.yml"]) + self.assertEqual(report.score_breakdown["sensitive_files"], 25) + self.assertEqual(report.score_breakdown["total"], report.score) def test_docs_are_ignored_from_size_budget(self): report = analyze([FileChange("docs/guide.md", 1000, 0)], DEFAULT_CONFIG) @@ -53,6 +65,57 @@ def test_root_markdown_is_ignored_by_recursive_pattern(self): self.assertEqual(report.changed_lines, 0) self.assertEqual(report.violations, []) + def test_path_rule_applies_stricter_limit(self): + config = { + **DEFAULT_CONFIG, + "path_rules": [ + { + "name": "migrations", + "patterns": ["**/migrations/**"], + "max_changed_lines": 20, + "require_tests_after_lines": 1, + } + ], + } + report = analyze([FileChange("app/migrations/001.sql", 30, 0)], config) + self.assertIn("[migrations] changed lines 30 exceed limit 20", report.violations) + self.assertIn("[migrations] tests required for changes of 1+ lines", report.violations) + self.assertEqual(report.path_rule_results[0]["name"], "migrations") + + def test_score_breakdown_accounts_for_cap(self): + report = analyze([FileChange("src/large.py", 2000, 0)], DEFAULT_CONFIG) + breakdown = report.score_breakdown + self.assertEqual(sum(value for key, value in breakdown.items() if key != "total"), 100) + self.assertEqual(breakdown["total"], 100) + + +class ConfigTests(unittest.TestCase): + def write_config(self, directory, config): + path = Path(directory) / "config.json" + path.write_text(json.dumps(config)) + return path + + def test_rejects_negative_numeric_threshold(self): + with tempfile.TemporaryDirectory() as directory: + path = self.write_config(directory, {"max_changed_lines": -1}) + with self.assertRaisesRegex(ValueError, "max_changed_lines"): + load_config(path) + + def test_rejects_boolean_numeric_threshold(self): + with tempfile.TemporaryDirectory() as directory: + path = self.write_config(directory, {"max_changed_files": True}) + with self.assertRaisesRegex(ValueError, "max_changed_files"): + load_config(path) + + def test_rejects_unknown_path_rule_key(self): + with tempfile.TemporaryDirectory() as directory: + path = self.write_config( + directory, + {"path_rules": [{"name": "api", "patterns": ["api/**"], "mystery": 1}]}, + ) + with self.assertRaisesRegex(ValueError, "unknown keys"): + load_config(path) + if __name__ == "__main__": unittest.main()