Add option to allow bots reviewing bot code - #68
Conversation
Concrete usecase for this is the backport PR AI review authored by bots.
PR SummaryMedium Risk Overview Preflight no longer drops all bot comment/review events: bots whose login appears in the parsed allowlist can enter the pipeline; others stay blocked before runners start. Authorization in the resolve step branches on The seidroid AI review README documents the new input, example caller config, and updated security notes. The assistant workflow is unchanged and still ignores bot comments. Reviewed by Cursor Bugbot for commit 67d7fbf. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds an allowed-bots allowlist so specific bot logins can request re-reviews; the authorization logic in resolve fails closed correctly (empty allowlist denies all bots, fork PRs still can't request explicit re-reviews, humans still go through the team check). The one concern is that removing user.type != 'Bot' from the preflight if: drops the cheap pre-filter for all bots rather than just non-allowlisted ones, so denied bot events — including seidroid's own reviews — now consume a preflight job.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Security docs: allowlisting a bot effectively grants re-review triggering to anyone who can make that bot post a comment (e.g.
github-actions[bot]can be driven by any workflow in the repo). Worth one sentence in the README security section telling operators to only list bots whose comment bodies are not attacker-controlled. - Comment drift: the block comment above the
resolvestep (.github/workflows/ai-review.yml:200-202) still says further reviews "require an exact command from an active member of allowed-team"; it was not updated for the new bot-allowlist path, unlike the preflight comment. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
@seidroid review |
There was a problem hiding this comment.
The bot allowlist is implemented correctly and fails closed at every layer (an empty array denies all bots, forks still cannot request explicit re-reviews, humans still go through the team check), and it resolves the earlier preflight-filter finding. The remaining notes are documentation and defensive-parsing polish.
Findings: 0 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Comment drift: the block comment above the
resolvestep (.github/workflows/ai-review.yml:213-215) still says further reviews "require an exact command from an active member of allowed-team" and that "Membership lookup failures deny access" — it was not updated for the new bot-allowlist path, unlike the preflight comment above it. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| ignores it. Human re-review requests are accepted only from active members of | ||
| `allowed-team`; bot requests require an exact, case-insensitive login match in | ||
| the `allowed-bots` JSON array. Non-allowlisted bots are rejected before a runner starts. | ||
| Membership lookup errors and empty allowlists fail closed. |
There was a problem hiding this comment.
[suggestion] This documents the mechanism but not its trust consequence: allowlisting a bot effectively grants re-review triggering to anyone who can make that bot post a comment. github-actions[bot], for example, can be driven by any workflow in the repo, so allowlisting it delegates the trigger to anyone who can add or modify a workflow. Worth one sentence telling operators to only list bots whose comment bodies are not attacker-controlled.
| } catch (error) { | ||
| core.notice(`Could not verify active membership for ${actor} in ${team}; denying request.`); | ||
| if (actorType === "Bot") { | ||
| const configuredBots = JSON.parse(process.env.ALLOWED_BOTS || "[]"); |
There was a problem hiding this comment.
[nit] JSON.parse is unguarded while the very next lines defensively handle a non-array result — the asymmetry is odd, since a malformed allowed-bots string is the more likely operator error and it throws here, failing the step rather than denying cleanly with a notice. A try/catch defaulting to [] (plus a core.notice) would match the fail-closed style used for the allowed-team branch.
Relatedly, fromJSON(inputs.allowed-bots) in the preflight if: has no fallback at all, so a malformed value errors the expression for any bot-authored comment event.
2 participants
Concrete usecase for this is the backport PR AI review authored by bots.