Trust an agent-review marker from anyone with write access, not only by author_association. Closes #137 - #138
Conversation
…by author_association. Closes #137 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
plamber
left a comment
plamber
left a comment
There was a problem hiding this comment.
No RED. Trust widening is sound; two AMBER to close before merge.
| Finding | Where | |
|---|---|---|
| 🟠 | Bots reach the permission lookup; skip it unless user.type === 'User' |
scripts/pr-agent-review.mjs:340 |
| 🟠 | Premise unproven with the callers' real contents: read token; add run evidence to the PR |
scripts/pr-agent-review.mjs:286 |
| ⚪ | role_name ?? permission rejects custom write-based roles; read permission |
scripts/pr-agent-review.mjs:297 |
| ⚪ | Comment claims bots get a 404; not documented | scripts/pr-agent-review.mjs:274 |
| ⚪ | Added readme sentences overrun the wrap | readme.md:106 |
Five findings posted; left out: MEMBER read-only over-trust (pre-existing), extra API cost (bounded, fails red), and a stub test for hasWriteAccess.
Good: every error path leaves the review untrusted, and trusted associations never trigger a lookup.
Generated by Claude Code
| const hasMarker = MARKER.test(review.body || ''); | ||
| MARKER.lastIndex = 0; | ||
| if (!hasMarker) continue; | ||
| if (await hasWriteAccess(entry.by)) all.push(entry); |
There was a problem hiding this comment.
🟠 App bots have association NONE/CONTRIBUTOR, so they now always reach this lookup. I expect a 404, but that is undocumented. Guard with review.user?.type === 'User' so bot handling stops depending on endpoint behaviour (claude[bot] posts PR-steered output via pr_code_review.yml).
| let result = false; | ||
| try { | ||
| const response = await fetch( | ||
| `${cfg.apiUrl}/repos/${owner}/${repo}/collaborators/${encodeURIComponent(login)}/permission`, |
There was a problem hiding this comment.
🟠 The endpoint's prose says the caller needs push access. Metadata:read is implicit, so it should work with contents: read, but that is unproven. If it 403s the fix silently does nothing. Put run evidence in the PR: a private member with write accepted, a read-only member rejected, using the callers' permissions block.
| ); | ||
| if (response.ok) { | ||
| const body = await response.json(); | ||
| result = WRITE_ROLES.has(body.role_name ?? body.permission); |
There was a problem hiding this comment.
⚪ role_name is always present, so ?? never falls back. A custom role based on write gets its own name, not in WRITE_ROLES, and is rejected. permission is the legacy field (admin/write/read/none; maintain→write, triage→read), so body.permission === 'admin' || body.permission === 'write' is as tight and has no false negatives.
| // 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 |
There was a problem hiding this comment.
⚪ "or a bot login" is not documented behaviour of this endpoint (GitHub documents it working for bots). Drop the claim; the type === 'User' guard is what excludes bots.
|
|
||
| **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 |
There was a problem hiding this comment.
⚪ The added sentences make this one very long line and the next line is not rewrapped. Rewrap the paragraph to the file's ~100 columns.
…ion field, and rewrap the readme. Closes #137 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
1 participant
Closes #137.
github.tokenis not an org member, so GitHub reports a private org member as CONTRIBUTOR andpr-agent-review.mjsdiscarded their review (Identity#1123, EasyLifeBertolla, org admin).A marker-bearing review from a person whose association is not trusted now falls back to
GET /collaborators/{login}/permission;admin/writecounts. One lookup per login, only for marker reviews, fails closed on 403/5xx/network errors (with a warning). Bot reviewers are never looked up.Verified
Run on a throwaway branch with the callers' exact permissions block (
contents: read,pull-requests: read,statuses: write), usinggithub.token:permissionadminadminreadnoneSo the token is allowed to call the endpoint, a private member with write is accepted, and read-only and bot logins are not. The probe branch is deleted.
Changed after review
review.user.type === 'User', readpermissioninstead ofrole_name, drop the undocumented bot claim from the comment, rewrap the readme paragraph. The AMBER about unproven token access is answered by the table above.Left unaddressed on purpose: read-only public members are already trusted by association (pre-existing), and a stub test for
hasWriteAccess(there is no test harness for this script yet).The red
call-workflow-lint / actionlintjob fails on every recent PR in this repo: actionlint 1.7.12 does not knowjob.workflow_sha, in files this PR does not touch. It is not a required check.🤖 Generated with Claude Code