feat: ACNA-4515 add pr-reviewer workflow#296
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 PR Reviewer
The workflow is generally well-structured with a reasonable permission gate. However, there are security concerns around comment body matching and injection risks from unsanitized GitHub context values interpolated directly into shell commands. The gate logic also lacks explicit handling for the case where the allowed output is not set (e.g., on error), which could lead to unexpected behavior.
📝 5 suggestion(s) - Please review inline comments below.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🤖 PR Reviewer
The diff addresses most previously raised injection and error-handling concerns: ISSUE_NUMBER and COMMENT_USER_LOGIN are now passed via env vars, and set -euo pipefail is present. A few issues remain, including the exact-match comment body check and direct interpolation of github.repository and github.event.pull_request.number/head.sha into shell strings.
📝 5 suggestion(s) (4 new, 1 re-raised)
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
There was a problem hiding this comment.
🤖 PR Reviewer
Most of the previously raised security issues (script injection via direct interpolation of github context values) have been addressed by moving values to env vars. The comment body exact-match issue remains unaddressed and could silently fail for users who add trailing whitespace or use different casing. The permission check logic also only allows 'admin' and 'maintain', excluding 'write' collaborators, which may be intentional but is worth confirming.
📝 2 suggestion(s) (1 new, 1 re-raised)
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🤖 PR Reviewer
The workflow is generally well-structured with good use of environment variables to avoid injection, proper error handling with set -euo pipefail, and clear inline comments addressing previous review concerns. The two previously raised issues have been partially addressed: the permission check now has the clarifying comment, but the comment body comparison is still exact-match only (though a NOTE comment was added documenting this limitation rather than fixing it).
🔄 1 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
Exact match is intentional. startsWith('/review') would match any comment beginning with /review — including /review-this-please, /reviewer, or /review123, silently triggering the bot on unintended input. The known limitation (trailing whitespace, mixed case) is a minor UX inconvenience that users quickly learn; the injection surface from startsWith is a worse trade-off. Documenting it via the comment is sufficient.
There was a problem hiding this comment.
🤖 PR Reviewer
The workflow logic and security gate are well-structured, but the previously raised issue with exact comment body matching remains unaddressed. The NOTE comment documents the fragility without resolving it, and GitHub Actions expression functions (trim, toLower, startsWith) are available and should be used here.
🔄 1 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| # NOTE: comment body matching is exact — /review or /pr-reviewer with no trailing spaces, newlines, or mixed case | ||
| if: | | ||
| (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == false) || | ||
| (github.event_name == 'issue_comment' && github.event.issue.pull_request != null && | ||
| (github.event.comment.body == '/review' || github.event.comment.body == '/pr-reviewer')) |
There was a problem hiding this comment.
[Re-raised] The NOTE comment documents the exact-match limitation but does not fix it. Trailing whitespace, newlines (common when users submit comments), or mixed casing will silently fail. GitHub Actions supports startsWith(), toLower(), and trim() — use them to make matching robust.
| # NOTE: comment body matching is exact — /review or /pr-reviewer with no trailing spaces, newlines, or mixed case | |
| if: | | |
| (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == false) || | |
| (github.event_name == 'issue_comment' && github.event.issue.pull_request != null && | |
| (github.event.comment.body == '/review' || github.event.comment.body == '/pr-reviewer')) | |
| # NOTE: comment body is trimmed and lowercased before matching to handle common whitespace/casing variations | |
| if: | | |
| (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == false) || | |
| (github.event_name == 'issue_comment' && github.event.issue.pull_request != null && | |
| (startsWith(trim(toLower(github.event.comment.body)), '/review') || startsWith(trim(toLower(github.event.comment.body)), '/pr-reviewer'))) |
There was a problem hiding this comment.
Exact match is intentional. startsWith('/review') would match any comment beginning with /review including /review-this-please, /reviewer, or /review123, silently triggering the bot on unintended input. The known limitation (trailing whitespace, mixed case) is a minor UX inconvenience, the injection surface from startsWith is a worse trade-off. Documenting it via the comment is sufficient.
There was a problem hiding this comment.
I encountered this in the template registry when I added "/update" and accidentally added a newline. Since it triggered the Github Action i left it for a while, not knowing that it skipped everything with no feedback. An annoyance, but I left it the same as you.
Exact match is intentional. startsWith('/review') would match any comment beginning with /review including /review-this-please, /reviewer, or /review123, silently triggering the bot on unintended input. The known limitation (trailing whitespace, mixed case) is a minor UX inconvenience, the injection surface from startsWith is a worse trade-off. Documenting it via the comment is sufficient.
| # NOTE: comment body matching is exact — /review or /pr-reviewer with no trailing spaces, newlines, or mixed case | ||
| if: | | ||
| (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == false) || | ||
| (github.event_name == 'issue_comment' && github.event.issue.pull_request != null && | ||
| (github.event.comment.body == '/review' || github.event.comment.body == '/pr-reviewer')) |
There was a problem hiding this comment.
I encountered this in the template registry when I added "/update" and accidentally added a newline. Since it triggered the Github Action i left it for a while, not knowing that it skipped everything with no feedback. An annoyance, but I left it the same as you.
3 participants
Description
Adds a GitHub Actions workflow that automatically reviews pull requests using Claude via AWS Bedrock. Reviews are posted as inline comments by github-actions[bot], with support for re-triggering via /review or /pr-reviewer comments.
Related Issue
ACNA-4515
Motivation and Context
Reduces manual review toil on routine PRs. The bot catches common issues (error handling, security, code clarity) and re-raises unresolved suggestions on subsequent pushes, so nothing gets lost across review cycles.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: