diff --git a/.github/workflows/pr-risk.yml b/.github/workflows/pr-risk.yml index 81d7dc7..7501ca9 100644 --- a/.github/workflows/pr-risk.yml +++ b/.github/workflows/pr-risk.yml @@ -3,19 +3,16 @@ name: PR Risk # Shadow-mode risk check for small pull requests. Comments whether the PR would have been # auto-approved and why; it never approves, so a human review is still what merges a PR. # -# A PR is skipped, with no comment, when it is a draft, comes from a fork, was not opened by an -# org member (bots included), changes more than 50 lines, or touches a deny-listed path -# (defaults in scripts/pr_risk.py plus the caller's `deny-paths`). Otherwise Claude classifies the diff and may deny it on its own judgment. +# A PR is skipped, with no comment, when it is a draft, comes from a fork, was opened by someone +# without write access (bots included), changes more than 50 lines, or touches a deny-listed +# path (defaults in scripts/pr_risk.py plus the caller's `deny-paths`). Otherwise Claude +# classifies the diff and may deny it on its own judgment. # If a PR that already has a comment becomes ineligible on a later push, the comment is # rewritten so it never shows a stale "yes". # # Credentials are split by step. The model runs with no tools and no GitHub token, reading -# only the PR data gathered with a read-only kernel-internal token. The token that can comment -# is minted after the model has finished, scoped to this repo and to pull-request writes. -# -# Gather uses the app rather than the workflow token because both the workflow token and the -# pull_request event payload report private org members as CONTRIBUTOR, and every Kernel -# member is private. An org-installed app's token sees the real MEMBER/OWNER association. +# only the PR data gathered with the caller's read-only token. The kernel-internal token is +# minted after the model has finished, scoped to this repo and to pull-request writes. on: workflow_call: @@ -65,22 +62,10 @@ jobs: curl -fsSL "$base/scripts/pr_risk.py" -o /tmp/pr_risk.py curl -fsSL "$base/.github/workflows/pr-risk/classify-prompt.md" -o /tmp/pr-risk-prompt.md - - name: Generate read token - id: read-token - # Pinned to a commit because this step is handed the app's private key. d72941d is v1.12.0. - uses: actions/create-github-app-token@d72941d797fd3113feb6b93fd0dec494b13a2547 - with: - app-id: ${{ secrets.ADMIN_APP_ID }} - private-key: ${{ secrets.ADMIN_APP_PRIVATE_KEY }} - owner: ${{ github.repository_owner }} - repositories: ${{ github.event.repository.name }} - permission-contents: read - permission-pull-requests: read - - name: Gather id: gather env: - GH_TOKEN: ${{ steps.read-token.outputs.token }} + GH_TOKEN: ${{ github.token }} DENY_PATHS: ${{ inputs.deny-paths }} run: python3 /tmp/pr_risk.py gather --repo "$REPO" --number "$NUMBER" --dir "$WORKDIR" diff --git a/README.md b/README.md index 9a38c4a..c03e971 100644 --- a/README.md +++ b/README.md @@ -71,9 +71,10 @@ Shadow-mode risk check for small PRs. When a PR is opened or marked ready, and o it leaves one comment saying whether the PR would have been auto-approved and why. It never approves. -These PRs are skipped with no comment: drafts, fork PRs, PRs not opened by an org member -(including bots), PRs over 50 changed lines, and PRs that touch a deny-listed path. The defaults live in `scripts/pr_risk.py`, and callers can add -more through `deny-paths`. Claude classifies everything else and can deny a PR on its own +These PRs are skipped with no comment: drafts, fork PRs, PRs whose author lacks write +access to the repo (including bots), PRs over 50 changed lines, and PRs that touch a +deny-listed path. The defaults live in `scripts/pr_risk.py`, and callers can add more +through `deny-paths`. Claude classifies everything else and can deny a PR on its own judgment. The comment stores its verdict in a hidden `pr-risk:v1` JSON marker for later analysis. ```yaml diff --git a/scripts/pr_risk.py b/scripts/pr_risk.py index 1236912..79e4c3d 100644 --- a/scripts/pr_risk.py +++ b/scripts/pr_risk.py @@ -117,6 +117,10 @@ def files(self, number: int) -> list[dict[str, Any]]: def comments(self, number: int) -> list[dict[str, Any]]: return self.paginate(f"/repos/{self.repo}/issues/{number}/comments?per_page=100") + def permission(self, login: str) -> str: + data = json.loads(self.api(f"/repos/{self.repo}/collaborators/{urllib.parse.quote(login)}/permission")) + return data["permission"] + def file_at(self, path: str, ref: str) -> str | None: try: return self.api("-H", "Accept: application/vnd.github.raw", @@ -160,17 +164,19 @@ def deny_patterns(extra: str) -> list[str]: return list(DEFAULT_DENY_PATHS) + [line for line in lines if line and not line.startswith("#")] -def ineligible_reason(pr: dict[str, Any], files: list[dict[str, Any]], patterns: list[str]) -> str | None: +def ineligible_reason(pr: dict[str, Any], files: list[dict[str, Any]], patterns: list[str], + author_permission: str) -> str | None: if pr.get("draft"): return "draft PR" head_repo = (pr["head"].get("repo") or {}).get("full_name") if head_repo != pr["base"]["repo"]["full_name"]: return "PR comes from a fork" - # Only org members' own changes are candidates. Bots are never members, so their PRs are - # skipped too: no person wrote them, and approving them unread is a separate decision. - # This is only right when read with the app token; see the note in pr-risk.yml. - if pr.get("author_association") not in ("OWNER", "MEMBER"): - return "author is not an org member" + # Only changes by people who can already push here are candidates. Bots have no repo + # permission, so their PRs are skipped too: no person wrote them, and approving them unread + # is a separate decision. author_association can't be used: every Kernel member is private, + # and both the workflow token and the event payload report private members as CONTRIBUTOR. + if author_permission not in ("admin", "write"): + return "author does not have write access" changed = pr["additions"] + pr["deletions"] if changed > MAX_CHANGED_LINES: return f"{changed} changed lines (limit {MAX_CHANGED_LINES})" @@ -286,7 +292,7 @@ def gather(gh: GitHub, number: int, extra_deny: str, out: Path) -> dict[str, Any pr = gh.pull(number) files = gh.files(number) existing = find_comment(gh.comments(number)) - reason = ineligible_reason(pr, files, deny_patterns(extra_deny)) + reason = ineligible_reason(pr, files, deny_patterns(extra_deny), gh.permission(pr["user"]["login"])) gate = { "sha": pr["head"]["sha"], "eligible": reason is None, diff --git a/scripts/test_pr_risk.py b/scripts/test_pr_risk.py index 21755cd..1a47203 100644 --- a/scripts/test_pr_risk.py +++ b/scripts/test_pr_risk.py @@ -29,6 +29,8 @@ sys.exit(1) if "/contents/" in path: print("raw file", end="") +elif path.endswith("/permission"): + print(json.dumps({"permission": "none", "role_name": ""})) elif "--slurp" in sys.argv: print(json.dumps([[{"n": 1}, {"n": 2}], [{"n": 3}]])) else: @@ -38,10 +40,10 @@ PATCH = "@@ -10,3 +10,4 @@ flags:\n - a\n - b\n+ - c\n - d" -def pr(additions=1, deletions=0, draft=False, fork=False, sha="abc1234def", association="MEMBER"): +def pr(additions=1, deletions=0, draft=False, fork=False, sha="abc1234def"): return { "number": 7, "draft": draft, "title": "Add an org to a flag", "body": "Adds one org.", - "user": {"login": "someone"}, "author_association": association, + "user": {"login": "someone"}, "author_association": "CONTRIBUTOR", "additions": additions, "deletions": deletions, "head": {"sha": sha, "repo": {"full_name": "external/infra" if fork else "kernel/infra"}}, "base": {"sha": "base000", "ref": "main", "repo": {"full_name": "kernel/infra"}}, @@ -54,12 +56,13 @@ def file(name="posthog/flags/prod.yaml", patch=PATCH, **extra): class FakeGitHub: - def __init__(self, pull, files, comments=(), contents=None, head_after=None): + def __init__(self, pull, files, comments=(), contents=None, head_after=None, permission="write"): self._pull = pull self._files = list(files) self._comments = list(comments) self._contents = contents or {} self.head_after = head_after + self._permission = permission self.created = [] self.updated = [] self.pull_reads = 0 @@ -76,6 +79,9 @@ def files(self, number): def comments(self, number): return self._comments + def permission(self, login): + return self._permission + def file_at(self, path, ref): return self._contents.get((path, ref)) @@ -130,21 +136,29 @@ def test_ineligible_reasons(self): renamed = file("b.yaml", status="renamed", previous_filename="a.yaml", additions=0, changes=0) del renamed["patch"] cases = { - "draft": (pr(draft=True), [file()]), - "fork": (pr(fork=True), [file()]), - "not an org member": (pr(association="CONTRIBUTOR"), [file()]), - "author is not an org member": (pr(association="NONE"), [file()]), - "51 changed lines": (pr(additions=30, deletions=21), [file()]), - "deny list (`identity/**`)": (pr(), [file("identity/access.go")]), - "deny list (`.github/**`)": (pr(), [file("ci.yml", previous_filename=".github/workflows/ci.yml")]), - "`bin/helper` has no reviewable text diff": (pr(additions=0), [binary]), - "`b.yaml` has no reviewable text diff": (pr(additions=0), [renamed]), + "draft": (pr(draft=True), [file()], "write"), + "fork": (pr(fork=True), [file()], "write"), + "author does not have write access": (pr(), [file()], "read"), + "does not have write access": (pr(), [file()], "none"), + "51 changed lines": (pr(additions=30, deletions=21), [file()], "write"), + "deny list (`identity/**`)": (pr(), [file("identity/access.go")], "write"), + "deny list (`.github/**`)": (pr(), [file("ci.yml", previous_filename=".github/workflows/ci.yml")], "write"), + "`bin/helper` has no reviewable text diff": (pr(additions=0), [binary], "write"), + "`b.yaml` has no reviewable text diff": (pr(additions=0), [renamed], "write"), } - for expected, (p, files) in cases.items(): + for expected, (p, files, permission) in cases.items(): with self.subTest(expected=expected): - self.assertIn(expected, pr_risk.ineligible_reason(p, files, patterns)) - self.assertIsNone(pr_risk.ineligible_reason(pr(additions=25, deletions=25), [file()], patterns)) - self.assertIsNone(pr_risk.ineligible_reason(pr(association="OWNER"), [file()], patterns)) + self.assertIn(expected, pr_risk.ineligible_reason(p, files, patterns, permission)) + # pr() reports CONTRIBUTOR, as the API does for every private member; it must not matter. + self.assertIsNone(pr_risk.ineligible_reason(pr(additions=25, deletions=25), [file()], patterns, "write")) + self.assertIsNone(pr_risk.ineligible_reason(pr(), [file()], patterns, "admin")) + + def test_bot_author_is_silent(self): + gh = FakeGitHub(pr(), [file()], permission="none") + gate, code, model_input = self.run_pr(gh) + self.assertEqual(gate["reason"], "author does not have write access") + self.assertIsNone(model_input) + self.assertEqual((gh.created, gh.updated), ([], [])) def test_ineligible_without_comment_is_silent(self): gh = FakeGitHub(pr(additions=200), [file()]) @@ -262,12 +276,14 @@ def test_github_client_calls_gh(self): self.assertEqual(gh.file_at("dir/a b.yaml", "abc"), "raw file") self.assertIsNone(gh.file_at("missing.yaml", "abc")) gh.update_comment(9, "hello") + self.assertEqual(gh.permission("kernel-internal[bot]"), "none") calls = json.loads((root / "log.json").read_text()) self.assertEqual(calls[0]["args"], ["api", "--paginate", "--slurp", "/repos/kernel/infra/issues/7/comments?per_page=100"]) self.assertIn("/repos/kernel/infra/contents/dir/a%20b.yaml?ref=abc", calls[1]["args"]) self.assertIn("Accept: application/vnd.github.raw", calls[1]["args"]) self.assertEqual(calls[3]["args"][:4], ["api", "-X", "PATCH", "/repos/kernel/infra/issues/comments/9"]) self.assertEqual(json.loads(calls[3]["stdin"]), {"body": "hello"}) + self.assertEqual(calls[4]["args"], ["api", "/repos/kernel/infra/collaborators/kernel-internal%5Bbot%5D/permission"]) def test_workflow_isolates_the_model(self): workflow = yaml.safe_load(WORKFLOW.read_text()) @@ -280,12 +296,7 @@ def test_workflow_isolates_the_model(self): names = list(steps) self.assertLess(names.index("Classify"), names.index("Generate app token")) self.assertEqual(steps["Generate app token"]["with"]["permission-pull-requests"], "write") - # The workflow token and event payload both report private members as CONTRIBUTOR. - self.assertEqual(steps["Gather"]["env"]["GH_TOKEN"], "${{ steps.read-token.outputs.token }}") - read = steps["Generate read token"]["with"] - self.assertEqual({k: v for k, v in read.items() if k.startswith("permission-")}, - {"permission-contents": "read", "permission-pull-requests": "read"}) - self.assertLess(names.index("Generate read token"), names.index("Gather")) + self.assertEqual(steps["Gather"]["env"]["GH_TOKEN"], "${{ github.token }}") triggers = workflow[True]["workflow_call"] self.assertEqual(set(triggers["secrets"]), {"ADMIN_APP_ID", "ADMIN_APP_PRIVATE_KEY", "ANTHROPIC_API_KEY"})