diff --git a/.github/workflows/pr-risk.yml b/.github/workflows/pr-risk.yml index 8bf009c..81d7dc7 100644 --- a/.github/workflows/pr-risk.yml +++ b/.github/workflows/pr-risk.yml @@ -10,8 +10,12 @@ name: PR Risk # 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 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. +# 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. on: workflow_call: @@ -61,12 +65,23 @@ 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: ${{ github.token }} + GH_TOKEN: ${{ steps.read-token.outputs.token }} DENY_PATHS: ${{ inputs.deny-paths }} - AUTHOR_ASSOCIATION: ${{ github.event.pull_request.author_association }} run: python3 /tmp/pr_risk.py gather --repo "$REPO" --number "$NUMBER" --dir "$WORKDIR" - name: Install Claude Code CLI diff --git a/scripts/pr_risk.py b/scripts/pr_risk.py index c6c16a3..1236912 100644 --- a/scripts/pr_risk.py +++ b/scripts/pr_risk.py @@ -160,8 +160,7 @@ 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], - association: str) -> str | None: +def ineligible_reason(pr: dict[str, Any], files: list[dict[str, Any]], patterns: list[str]) -> str | None: if pr.get("draft"): return "draft PR" head_repo = (pr["head"].get("repo") or {}).get("full_name") @@ -169,9 +168,8 @@ def ineligible_reason(pr: dict[str, Any], files: list[dict[str, Any]], patterns: 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. - # The association comes from the event payload: the API answers for the caller's token, - # and the workflow token cannot see private org membership, so it reports CONTRIBUTOR. - if association not in ("OWNER", "MEMBER"): + # 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" changed = pr["additions"] + pr["deletions"] if changed > MAX_CHANGED_LINES: @@ -283,12 +281,12 @@ def previous_data(body: str) -> dict[str, Any] | None: return None -def gather(gh: GitHub, number: int, extra_deny: str, association: str, out: Path) -> dict[str, Any]: +def gather(gh: GitHub, number: int, extra_deny: str, out: Path) -> dict[str, Any]: out.mkdir(parents=True, exist_ok=True) pr = gh.pull(number) files = gh.files(number) existing = find_comment(gh.comments(number)) - reason = ineligible_reason(pr, files, deny_patterns(extra_deny), association) + reason = ineligible_reason(pr, files, deny_patterns(extra_deny)) gate = { "sha": pr["head"]["sha"], "eligible": reason is None, @@ -439,7 +437,7 @@ def main() -> int: return 0 gh = GitHub(args.repo) if args.cmd == "gather": - gather(gh, args.number, os.environ.get("DENY_PATHS", ""), os.environ["AUTHOR_ASSOCIATION"], args.dir) + gather(gh, args.number, os.environ.get("DENY_PATHS", ""), args.dir) return 0 return comment(gh, args.number, args.dir, os.environ.get("RUN_URL", "")) diff --git a/scripts/test_pr_risk.py b/scripts/test_pr_risk.py index 09a44c9..21755cd 100644 --- a/scripts/test_pr_risk.py +++ b/scripts/test_pr_risk.py @@ -38,10 +38,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"): +def pr(additions=1, deletions=0, draft=False, fork=False, sha="abc1234def", association="MEMBER"): return { "number": 7, "draft": draft, "title": "Add an org to a flag", "body": "Adds one org.", - "user": {"login": "someone"}, "author_association": "CONTRIBUTOR", + "user": {"login": "someone"}, "author_association": association, "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"}}, @@ -97,10 +97,10 @@ def visible_words(body): class PRRiskTests(unittest.TestCase): - def run_pr(self, gh, verdict=VERDICT, result=None, extra_deny="", association="MEMBER"): + def run_pr(self, gh, verdict=VERDICT, result=None, extra_deny=""): with tempfile.TemporaryDirectory() as tmp: work = Path(tmp) - gate = pr_risk.gather(gh, 7, extra_deny, association, work) + gate = pr_risk.gather(gh, 7, extra_deny, work) if gate["eligible"]: (work / "result.json").write_text(json.dumps(result or {"is_error": False, "structured_output": verdict})) code = pr_risk.comment(gh, 7, work, "https://example.com/run") @@ -130,22 +130,21 @@ 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()], "MEMBER"), - "fork": (pr(fork=True), [file()], "MEMBER"), - "not an org member": (pr(), [file()], "CONTRIBUTOR"), - "author is not an org member": (pr(), [file()], "NONE"), - "51 changed lines": (pr(additions=30, deletions=21), [file()], "MEMBER"), - "deny list (`identity/**`)": (pr(), [file("identity/access.go")], "MEMBER"), - "deny list (`.github/**`)": (pr(), [file("ci.yml", previous_filename=".github/workflows/ci.yml")], "MEMBER"), - "`bin/helper` has no reviewable text diff": (pr(additions=0), [binary], "MEMBER"), - "`b.yaml` has no reviewable text diff": (pr(additions=0), [renamed], "MEMBER"), + "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]), } - for expected, (p, files, association) in cases.items(): + for expected, (p, files) in cases.items(): with self.subTest(expected=expected): - self.assertIn(expected, pr_risk.ineligible_reason(p, files, patterns, association)) - # The API reports CONTRIBUTOR for private members; only the event payload's value counts. - self.assertIsNone(pr_risk.ineligible_reason(pr(additions=25, deletions=25), [file()], patterns, "MEMBER")) - self.assertIsNone(pr_risk.ineligible_reason(pr(), [file()], patterns, "OWNER")) + 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)) def test_ineligible_without_comment_is_silent(self): gh = FakeGitHub(pr(additions=200), [file()]) @@ -281,8 +280,12 @@ 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") - self.assertEqual(steps["Gather"]["env"]["AUTHOR_ASSOCIATION"], - "${{ github.event.pull_request.author_association }}") + # 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")) triggers = workflow[True]["workflow_call"] self.assertEqual(set(triggers["secrets"]), {"ADMIN_APP_ID", "ADMIN_APP_PRIVATE_KEY", "ANTHROPIC_API_KEY"})