Trust an agent-review marker from anyone with write access, not only by author_association. Closes #137

.github/workflows/pr_agent_review.yml

@@ -1,3 +1,3 @@
 # Verifies a Claude review exists for the pull request.
 #
 # The approval rule is "one human approval plus a passing agent review on every pull request".
@@ -75,7 +75,9 @@
 #
 # WHO CAN SATISFY THIS CHECK
 #
-# Only a REVIEW, only from OWNER / MEMBER / COLLABORATOR. Plain issue comments are not read.
+# Only a REVIEW, only from OWNER / MEMBER / COLLABORATOR, or from anyone with write access to the
+# repository. The second route exists because github.token reports a PRIVATE org member as
+# CONTRIBUTOR, so author_association alone rejects real maintainers. Plain issue comments are not read.
 # The author of a pull request may satisfy it on their own pull request; the status names who
 # posted the marker so an approver can see that and re-run it if they want.
 #

readme.md

@@ -103,7 +103,10 @@ while the pull request itself receives nothing and stays red. That is why ours c
 prefix rather than sitting next to it as `review`.
 
 **Who can satisfy it.** The marker is accepted only from a **review** (not a plain comment) by
-someone with `OWNER`, `MEMBER` or `COLLABORATOR` standing. A pull request author *may* satisfy it
+someone with `OWNER`, `MEMBER` or `COLLABORATOR` standing, or with write access to the
+repository. The second route is there because the check reads with `github.token`, which sees a
+*private* org member as `CONTRIBUTOR`; the reviewer's repository permission does not depend on
+whether their membership is public. A pull request author *may* satisfy it
 on their own pull request — running `/el-review` before asking anyone to look is a good habit and
 banning it would only discourage it — but the status names who posted it, so an approver sees a
 self-review and can re-run it. The human approval is a separate person regardless; GitHub

scripts/pr-agent-review.mjs

@@ -260,6 +260,54 @@ if (cfg.exemptAuthors.includes(author)) {
 // separate person either way, which GitHub enforces.
 const TRUSTED_ASSOCIATIONS = new Set(['OWNER', 'MEMBER', 'COLLABORATOR']);
 
+// author_association is NOT a reliable statement of standing when read with github.token. The
+// token is an app installation, not an org member, so GitHub reports a PRIVATE org member as
+// CONTRIBUTOR (or NONE) to it, even when that person is an org admin. Their review then looked
+// untrusted and the check stayed red however many times /el-review ran.
+//
+// The repository permission does not have that blind spot: it is a property of the repository,
+// not of who may see someone's membership. So an untrusted association is not the end of the
+// question for a review that carries a marker -- the reviewer's own permission on this repository
+// is asked, and write or above counts, which is what "OWNER, MEMBER or COLLABORATOR" was standing
+// in for.
+//
+// Fails closed: a 404 (not a collaborator), a 403 or any other error leaves the
+// review untrusted, and the warning below says so. Only reviews that carry a marker are looked up,
+// and once per login, so a pull request nobody has reviewed costs no extra calls.
+// `permission` is the legacy field: admin, write, read or none, with maintain folded into write
+// and triage into read. Unlike `role_name` it stays meaningful for a custom repository role.
+const WRITE_PERMISSIONS = new Set(['admin', 'write']);
+const permissionCache = new Map();
+
+async function hasWriteAccess(login) {
+  if (!login) return false;
+  if (!permissionCache.has(login)) {
+    let result = false;
+    try {
+      const response = await fetch(
+        `${cfg.apiUrl}/repos/${owner}/${repo}/collaborators/${encodeURIComponent(login)}/permission`,
+        {
+          headers: {
+            authorization: `Bearer ${cfg.token}`,
+            accept: 'application/vnd.github+json',
+            'x-github-api-version': '2022-11-28',
+          },
+        },
+      );
+      if (response.ok) {
+        const body = await response.json();
+        result = WRITE_PERMISSIONS.has(body.permission);
+      } else if (response.status !== 404) {
+        console.warn(`::warning::Could not read the repository permission of @${login}: ${response.status}`);
+      }
+    } catch (error) {
+      console.warn(`::warning::Could not read the repository permission of @${login}: ${error.message}`);
+    }
+    permissionCache.set(login, result);
+  }
+  return permissionCache.get(login);
+}
+
 const reviews = await apiAll(`/repos/${owner}/${repo}/pulls/${cfg.prNumber}/reviews`);
 
 // Kept separately so an untrusted marker can be reported rather than silently ignored. Someone
@@ -284,9 +332,17 @@ for (const review of reviews) {
     // review.
     sha: review.commit_id,
   };
-  if (TRUSTED_ASSOCIATIONS.has(review.author_association)) all.push(entry);
-  else if (MARKER.test(review.body || '')) untrusted.push(entry);
+  if (TRUSTED_ASSOCIATIONS.has(review.author_association)) {
+    all.push(entry);
+    continue;
+  }
+  const hasMarker = MARKER.test(review.body || '');
   MARKER.lastIndex = 0;
+  if (!hasMarker) continue;
+  // Only a person is looked up. An app's bot login is never trusted by this route, so that does
+  // not depend on how the permission endpoint treats one.
+  if (review.user?.type === 'User' && (await hasWriteAccess(entry.by))) all.push(entry);
+  else untrusted.push(entry);
 }
 
 // Every trusted marker counts, whatever commit its review was recorded against. The one for the
@@ -367,7 +423,7 @@ console.warn(`::warning::${description}`);
 if (untrusted.length > 0) {
   console.warn(`::warning::Ignored ${untrusted.length} marker(s) from an author without write access.`);
   for (const item of untrusted) {
-    console.warn(`  - @${item.by || 'unknown'} (${item.association}) -- needs OWNER, MEMBER or COLLABORATOR`);
+    console.warn(`  - @${item.by || 'unknown'} (${item.association}) -- needs OWNER, MEMBER or COLLABORATOR, or write access to this repository`);
   }
   console.warn('');
 }