From aaf18d23d59484ba703046c487695cfaf77e6d52 Mon Sep 17 00:00:00 2001 From: Patrick Lamber Date: Wed, 30 Sep 2026 19:05:00 +0200 Subject: [PATCH 1/2] Trust an agent-review marker from anyone with write access, not only by author_association. Closes #137 Co-Authored-By: Claude Sonnet 5.5 --- .github/workflows/pr_agent_review.yml | 4 +- readme.md | 2 +- scripts/pr-agent-review.mjs | 58 +++++++++++++++++++++++++-- 3 files changed, 59 insertions(+), 5 deletions(-) diff --git a/.github/workflows/pr_agent_review.yml b/.github/workflows/pr_agent_review.yml index f163167..ae6d7dc 100644 --- a/.github/workflows/pr_agent_review.yml +++ b/.github/workflows/pr_agent_review.yml @@ -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. # diff --git a/readme.md b/readme.md index 60bf1a4..c9e229a 100644 --- a/readme.md +++ b/readme.md @@ -103,7 +103,7 @@ 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 diff --git a/scripts/pr-agent-review.mjs b/scripts/pr-agent-review.mjs index 95ec1f7..05c4d23 100644 --- a/scripts/pr-agent-review.mjs +++ b/scripts/pr-agent-review.mjs @@ -260,6 +260,52 @@ 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, or a bot login), 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. +const WRITE_ROLES = new Set(['admin', 'maintain', '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_ROLES.has(body.role_name ?? 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 +330,15 @@ 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; + if (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 +419,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(''); } From d096c7ba58b14e9c9e2e18157bbc4333b3b54b23 Mon Sep 17 00:00:00 2001 From: Patrick Lamber Date: Wed, 30 Sep 2026 19:13:07 +0200 Subject: [PATCH 2/2] Skip the permission lookup for bot reviewers, read the legacy permission field, and rewrap the readme. Closes #137 Co-Authored-By: Claude Sonnet 5.5 --- readme.md | 5 ++++- scripts/pr-agent-review.mjs | 12 ++++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/readme.md b/readme.md index c9e229a..479c8f9 100644 --- a/readme.md +++ b/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, 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 +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 diff --git a/scripts/pr-agent-review.mjs b/scripts/pr-agent-review.mjs index 05c4d23..63d245b 100644 --- a/scripts/pr-agent-review.mjs +++ b/scripts/pr-agent-review.mjs @@ -271,10 +271,12 @@ const TRUSTED_ASSOCIATIONS = new Set(['OWNER', 'MEMBER', 'COLLABORATOR']); // 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, or a bot login), a 403 or any other error leaves the +// 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. -const WRITE_ROLES = new Set(['admin', 'maintain', 'write']); +// `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) { @@ -294,7 +296,7 @@ async function hasWriteAccess(login) { ); if (response.ok) { const body = await response.json(); - result = WRITE_ROLES.has(body.role_name ?? body.permission); + result = WRITE_PERMISSIONS.has(body.permission); } else if (response.status !== 404) { console.warn(`::warning::Could not read the repository permission of @${login}: ${response.status}`); } @@ -337,7 +339,9 @@ for (const review of reviews) { const hasMarker = MARKER.test(review.body || ''); MARKER.lastIndex = 0; if (!hasMarker) continue; - if (await hasWriteAccess(entry.by)) all.push(entry); + // 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); }