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..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. 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..63d245b 100644 --- a/scripts/pr-agent-review.mjs +++ b/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(''); }