Skip to content

Commit b19d002

Browse files
authored
Check the PR author's repo permission instead of org association (#28)
* Check the PR author's repo permission instead of org association * Tidy PR risk skip wording --------- Co-authored-by: tnsardesai <18272584+tnsardesai@users.noreply.github.com>
1 parent 0eeb376 commit b19d002

4 files changed

Lines changed: 57 additions & 54 deletions

File tree

‎.github/workflows/pr-risk.yml‎

Lines changed: 7 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3,19 +3,16 @@ name: PR Risk
33
# Shadow-mode risk check for small pull requests. Comments whether the PR would have been
44
# auto-approved and why; it never approves, so a human review is still what merges a PR.
55
#
6-
# A PR is skipped, with no comment, when it is a draft, comes from a fork, was not opened by an
7-
# org member (bots included), changes more than 50 lines, or touches a deny-listed path
8-
# (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.
6+
# A PR is skipped, with no comment, when it is a draft, comes from a fork, was opened by someone
7+
# without write access (bots included), changes more than 50 lines, or touches a deny-listed
8+
# path (defaults in scripts/pr_risk.py plus the caller's `deny-paths`). Otherwise Claude
9+
# classifies the diff and may deny it on its own judgment.
910
# If a PR that already has a comment becomes ineligible on a later push, the comment is
1011
# rewritten so it never shows a stale "yes".
1112
#
1213
# Credentials are split by step. The model runs with no tools and no GitHub token, reading
13-
# only the PR data gathered with a read-only kernel-internal token. The token that can comment
14-
# is minted after the model has finished, scoped to this repo and to pull-request writes.
15-
#
16-
# Gather uses the app rather than the workflow token because both the workflow token and the
17-
# pull_request event payload report private org members as CONTRIBUTOR, and every Kernel
18-
# member is private. An org-installed app's token sees the real MEMBER/OWNER association.
14+
# only the PR data gathered with the caller's read-only token. The kernel-internal token is
15+
# minted after the model has finished, scoped to this repo and to pull-request writes.
1916

2017
on:
2118
workflow_call:
@@ -65,22 +62,10 @@ jobs:
6562
curl -fsSL "$base/scripts/pr_risk.py" -o /tmp/pr_risk.py
6663
curl -fsSL "$base/.github/workflows/pr-risk/classify-prompt.md" -o /tmp/pr-risk-prompt.md
6764
68-
- name: Generate read token
69-
id: read-token
70-
# Pinned to a commit because this step is handed the app's private key. d72941d is v1.12.0.
71-
uses: actions/create-github-app-token@d72941d797fd3113feb6b93fd0dec494b13a2547
72-
with:
73-
app-id: ${{ secrets.ADMIN_APP_ID }}
74-
private-key: ${{ secrets.ADMIN_APP_PRIVATE_KEY }}
75-
owner: ${{ github.repository_owner }}
76-
repositories: ${{ github.event.repository.name }}
77-
permission-contents: read
78-
permission-pull-requests: read
79-
8065
- name: Gather
8166
id: gather
8267
env:
83-
GH_TOKEN: ${{ steps.read-token.outputs.token }}
68+
GH_TOKEN: ${{ github.token }}
8469
DENY_PATHS: ${{ inputs.deny-paths }}
8570
run: python3 /tmp/pr_risk.py gather --repo "$REPO" --number "$NUMBER" --dir "$WORKDIR"
8671

‎README.md‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,10 @@ Shadow-mode risk check for small PRs. When a PR is opened or marked ready, and o
7171
it leaves one comment saying whether the PR would have been auto-approved and why. It never
7272
approves.
7373

74-
These PRs are skipped with no comment: drafts, fork PRs, PRs not opened by an org member
75-
(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
76-
more through `deny-paths`. Claude classifies everything else and can deny a PR on its own
74+
These PRs are skipped with no comment: drafts, fork PRs, PRs whose author lacks write
75+
access to the repo (including bots), PRs over 50 changed lines, and PRs that touch a
76+
deny-listed path. The defaults live in `scripts/pr_risk.py`, and callers can add more
77+
through `deny-paths`. Claude classifies everything else and can deny a PR on its own
7778
judgment. The comment stores its verdict in a hidden `pr-risk:v1` JSON marker for later analysis.
7879

7980
```yaml

‎scripts/pr_risk.py‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,10 @@ def files(self, number: int) -> list[dict[str, Any]]:
117117
def comments(self, number: int) -> list[dict[str, Any]]:
118118
return self.paginate(f"/repos/{self.repo}/issues/{number}/comments?per_page=100")
119119

120+
def permission(self, login: str) -> str:
121+
data = json.loads(self.api(f"/repos/{self.repo}/collaborators/{urllib.parse.quote(login)}/permission"))
122+
return data["permission"]
123+
120124
def file_at(self, path: str, ref: str) -> str | None:
121125
try:
122126
return self.api("-H", "Accept: application/vnd.github.raw",
@@ -160,17 +164,19 @@ def deny_patterns(extra: str) -> list[str]:
160164
return list(DEFAULT_DENY_PATHS) + [line for line in lines if line and not line.startswith("#")]
161165

162166

163-
def ineligible_reason(pr: dict[str, Any], files: list[dict[str, Any]], patterns: list[str]) -> str | None:
167+
def ineligible_reason(pr: dict[str, Any], files: list[dict[str, Any]], patterns: list[str],
168+
author_permission: str) -> str | None:
164169
if pr.get("draft"):
165170
return "draft PR"
166171
head_repo = (pr["head"].get("repo") or {}).get("full_name")
167172
if head_repo != pr["base"]["repo"]["full_name"]:
168173
return "PR comes from a fork"
169-
# Only org members' own changes are candidates. Bots are never members, so their PRs are
170-
# skipped too: no person wrote them, and approving them unread is a separate decision.
171-
# This is only right when read with the app token; see the note in pr-risk.yml.
172-
if pr.get("author_association") not in ("OWNER", "MEMBER"):
173-
return "author is not an org member"
174+
# Only changes by people who can already push here are candidates. Bots have no repo
175+
# permission, so their PRs are skipped too: no person wrote them, and approving them unread
176+
# is a separate decision. author_association can't be used: every Kernel member is private,
177+
# and both the workflow token and the event payload report private members as CONTRIBUTOR.
178+
if author_permission not in ("admin", "write"):
179+
return "author does not have write access"
174180
changed = pr["additions"] + pr["deletions"]
175181
if changed > MAX_CHANGED_LINES:
176182
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
286292
pr = gh.pull(number)
287293
files = gh.files(number)
288294
existing = find_comment(gh.comments(number))
289-
reason = ineligible_reason(pr, files, deny_patterns(extra_deny))
295+
reason = ineligible_reason(pr, files, deny_patterns(extra_deny), gh.permission(pr["user"]["login"]))
290296
gate = {
291297
"sha": pr["head"]["sha"],
292298
"eligible": reason is None,

‎scripts/test_pr_risk.py‎

Lines changed: 33 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@
2929
sys.exit(1)
3030
if "/contents/" in path:
3131
print("raw file", end="")
32+
elif path.endswith("/permission"):
33+
print(json.dumps({"permission": "none", "role_name": ""}))
3234
elif "--slurp" in sys.argv:
3335
print(json.dumps([[{"n": 1}, {"n": 2}], [{"n": 3}]]))
3436
else:
@@ -38,10 +40,10 @@
3840
PATCH = "@@ -10,3 +10,4 @@ flags:\n - a\n - b\n+ - c\n - d"
3941

4042

41-
def pr(additions=1, deletions=0, draft=False, fork=False, sha="abc1234def", association="MEMBER"):
43+
def pr(additions=1, deletions=0, draft=False, fork=False, sha="abc1234def"):
4244
return {
4345
"number": 7, "draft": draft, "title": "Add an org to a flag", "body": "Adds one org.",
44-
"user": {"login": "someone"}, "author_association": association,
46+
"user": {"login": "someone"}, "author_association": "CONTRIBUTOR",
4547
"additions": additions, "deletions": deletions,
4648
"head": {"sha": sha, "repo": {"full_name": "external/infra" if fork else "kernel/infra"}},
4749
"base": {"sha": "base000", "ref": "main", "repo": {"full_name": "kernel/infra"}},
@@ -54,12 +56,13 @@ def file(name="posthog/flags/prod.yaml", patch=PATCH, **extra):
5456

5557

5658
class FakeGitHub:
57-
def __init__(self, pull, files, comments=(), contents=None, head_after=None):
59+
def __init__(self, pull, files, comments=(), contents=None, head_after=None, permission="write"):
5860
self._pull = pull
5961
self._files = list(files)
6062
self._comments = list(comments)
6163
self._contents = contents or {}
6264
self.head_after = head_after
65+
self._permission = permission
6366
self.created = []
6467
self.updated = []
6568
self.pull_reads = 0
@@ -76,6 +79,9 @@ def files(self, number):
7679
def comments(self, number):
7780
return self._comments
7881

82+
def permission(self, login):
83+
return self._permission
84+
7985
def file_at(self, path, ref):
8086
return self._contents.get((path, ref))
8187

@@ -130,21 +136,29 @@ def test_ineligible_reasons(self):
130136
renamed = file("b.yaml", status="renamed", previous_filename="a.yaml", additions=0, changes=0)
131137
del renamed["patch"]
132138
cases = {
133-
"draft": (pr(draft=True), [file()]),
134-
"fork": (pr(fork=True), [file()]),
135-
"not an org member": (pr(association="CONTRIBUTOR"), [file()]),
136-
"author is not an org member": (pr(association="NONE"), [file()]),
137-
"51 changed lines": (pr(additions=30, deletions=21), [file()]),
138-
"deny list (`identity/**`)": (pr(), [file("identity/access.go")]),
139-
"deny list (`.github/**`)": (pr(), [file("ci.yml", previous_filename=".github/workflows/ci.yml")]),
140-
"`bin/helper` has no reviewable text diff": (pr(additions=0), [binary]),
141-
"`b.yaml` has no reviewable text diff": (pr(additions=0), [renamed]),
139+
"draft": (pr(draft=True), [file()], "write"),
140+
"fork": (pr(fork=True), [file()], "write"),
141+
"author does not have write access": (pr(), [file()], "read"),
142+
"does not have write access": (pr(), [file()], "none"),
143+
"51 changed lines": (pr(additions=30, deletions=21), [file()], "write"),
144+
"deny list (`identity/**`)": (pr(), [file("identity/access.go")], "write"),
145+
"deny list (`.github/**`)": (pr(), [file("ci.yml", previous_filename=".github/workflows/ci.yml")], "write"),
146+
"`bin/helper` has no reviewable text diff": (pr(additions=0), [binary], "write"),
147+
"`b.yaml` has no reviewable text diff": (pr(additions=0), [renamed], "write"),
142148
}
143-
for expected, (p, files) in cases.items():
149+
for expected, (p, files, permission) in cases.items():
144150
with self.subTest(expected=expected):
145-
self.assertIn(expected, pr_risk.ineligible_reason(p, files, patterns))
146-
self.assertIsNone(pr_risk.ineligible_reason(pr(additions=25, deletions=25), [file()], patterns))
147-
self.assertIsNone(pr_risk.ineligible_reason(pr(association="OWNER"), [file()], patterns))
151+
self.assertIn(expected, pr_risk.ineligible_reason(p, files, patterns, permission))
152+
# pr() reports CONTRIBUTOR, as the API does for every private member; it must not matter.
153+
self.assertIsNone(pr_risk.ineligible_reason(pr(additions=25, deletions=25), [file()], patterns, "write"))
154+
self.assertIsNone(pr_risk.ineligible_reason(pr(), [file()], patterns, "admin"))
155+
156+
def test_bot_author_is_silent(self):
157+
gh = FakeGitHub(pr(), [file()], permission="none")
158+
gate, code, model_input = self.run_pr(gh)
159+
self.assertEqual(gate["reason"], "author does not have write access")
160+
self.assertIsNone(model_input)
161+
self.assertEqual((gh.created, gh.updated), ([], []))
148162

149163
def test_ineligible_without_comment_is_silent(self):
150164
gh = FakeGitHub(pr(additions=200), [file()])
@@ -262,12 +276,14 @@ def test_github_client_calls_gh(self):
262276
self.assertEqual(gh.file_at("dir/a b.yaml", "abc"), "raw file")
263277
self.assertIsNone(gh.file_at("missing.yaml", "abc"))
264278
gh.update_comment(9, "hello")
279+
self.assertEqual(gh.permission("kernel-internal[bot]"), "none")
265280
calls = json.loads((root / "log.json").read_text())
266281
self.assertEqual(calls[0]["args"], ["api", "--paginate", "--slurp", "/repos/kernel/infra/issues/7/comments?per_page=100"])
267282
self.assertIn("/repos/kernel/infra/contents/dir/a%20b.yaml?ref=abc", calls[1]["args"])
268283
self.assertIn("Accept: application/vnd.github.raw", calls[1]["args"])
269284
self.assertEqual(calls[3]["args"][:4], ["api", "-X", "PATCH", "/repos/kernel/infra/issues/comments/9"])
270285
self.assertEqual(json.loads(calls[3]["stdin"]), {"body": "hello"})
286+
self.assertEqual(calls[4]["args"], ["api", "/repos/kernel/infra/collaborators/kernel-internal%5Bbot%5D/permission"])
271287

272288
def test_workflow_isolates_the_model(self):
273289
workflow = yaml.safe_load(WORKFLOW.read_text())
@@ -280,12 +296,7 @@ def test_workflow_isolates_the_model(self):
280296
names = list(steps)
281297
self.assertLess(names.index("Classify"), names.index("Generate app token"))
282298
self.assertEqual(steps["Generate app token"]["with"]["permission-pull-requests"], "write")
283-
# The workflow token and event payload both report private members as CONTRIBUTOR.
284-
self.assertEqual(steps["Gather"]["env"]["GH_TOKEN"], "${{ steps.read-token.outputs.token }}")
285-
read = steps["Generate read token"]["with"]
286-
self.assertEqual({k: v for k, v in read.items() if k.startswith("permission-")},
287-
{"permission-contents": "read", "permission-pull-requests": "read"})
288-
self.assertLess(names.index("Generate read token"), names.index("Gather"))
299+
self.assertEqual(steps["Gather"]["env"]["GH_TOKEN"], "${{ github.token }}")
289300
triggers = workflow[True]["workflow_call"]
290301
self.assertEqual(set(triggers["secrets"]), {"ADMIN_APP_ID", "ADMIN_APP_PRIVATE_KEY", "ANTHROPIC_API_KEY"})
291302

0 commit comments

Comments
 (0)