Conversation
Adds the review-enforcement workflow from the API repo to enforce that PRs have at least one approval from a non-bot reviewer, and to auto-request reviews from the product team when Dependabot CI fails. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
john-weiler
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #490 +/- ##
=======================================
Coverage ? 71.29%
=======================================
Files ? 71
Lines ? 5947
Branches ? 1480
=======================================
Hits ? 4240
Misses ? 1697
Partials ? 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds a GitHub Actions workflow to enforce PR review requirements and to auto-request review on Dependabot CI failures, aligning this repo’s PR process with the API repo’s policy.
Changes:
- Introduces
review-enforcement.ymlworkflow to enforce a minimum approval rule (excludingbaz-reviewer). - Skips enforcement for Dependabot PRs.
- On failed “Run Tests” workflow runs for Dependabot PRs, requests review from a random
rungalileo/productteam member.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ); | ||
|
|
||
| if (!hasNonBazApproval) { | ||
| core.setFailed('At least one approval from a non-baz-reviewer is required.'); |
There was a problem hiding this comment.
The failure message says “non-baz-reviewer”, but the logic also enforces “non-bot” via r.user.type === 'User'. Consider updating the message to reflect the actual requirement (human approval, excluding baz-reviewer) so it’s actionable for contributors.
| core.setFailed('At least one approval from a non-baz-reviewer is required.'); | |
| core.setFailed("At least one approval from a human reviewer other than 'baz-reviewer' is required."); |
| const pr = context.payload.pull_request; | ||
| const event = context.eventName; | ||
| const isDependabot = pr.user?.login === 'dependabot[bot]'; | ||
|
|
There was a problem hiding this comment.
enforce-review runs for workflow_run events too, but this script assumes context.payload.pull_request exists. On workflow_run, pull_request is undefined, so pr.user?.login will throw before the optional chain is evaluated. Add a job-level if: github.event_name != 'workflow_run' (or handle workflow_run explicitly by resolving the PR from context.payload.workflow_run.pull_requests and fetching PR data).
| const pr = context.payload.pull_request; | ||
| const event = context.eventName; | ||
| const isDependabot = pr.user?.login === 'dependabot[bot]'; | ||
|
|
||
| // Skip for Dependabot on all events | ||
| if (isDependabot) { | ||
| core.info('Dependabot PR detected - skipping review enforcement.'); | ||
| return; | ||
| } | ||
|
|
||
| // Only enforce review requirement on pull_request_review events | ||
| if (event === 'pull_request') { | ||
| core.info('PR opened/synchronized - waiting for review, not enforcing yet.'); | ||
| return; | ||
| } | ||
|
|
||
| // From here on, we know it's a pull_request_review event (submitted or dismissed): enforce rule | ||
| core.info('Review event detected - checking current approval status...'); | ||
|
|
||
| const reviews = await github.rest.pulls.listReviews({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: pr.number |
There was a problem hiding this comment.
On pull_request (opened/synchronize/reopened) this job exits successfully without checking approvals. If this workflow is used as a required status check, it can allow merging without approval and also won’t re-validate after new commits (when approvals may be dismissed). Consider running the approval check on pull_request events as well and failing when no valid approval exists.
| const pr = context.payload.pull_request; | |
| const event = context.eventName; | |
| const isDependabot = pr.user?.login === 'dependabot[bot]'; | |
| // Skip for Dependabot on all events | |
| if (isDependabot) { | |
| core.info('Dependabot PR detected - skipping review enforcement.'); | |
| return; | |
| } | |
| // Only enforce review requirement on pull_request_review events | |
| if (event === 'pull_request') { | |
| core.info('PR opened/synchronized - waiting for review, not enforcing yet.'); | |
| return; | |
| } | |
| // From here on, we know it's a pull_request_review event (submitted or dismissed): enforce rule | |
| core.info('Review event detected - checking current approval status...'); | |
| const reviews = await github.rest.pulls.listReviews({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| pull_number: pr.number | |
| const event = context.eventName; | |
| const { owner, repo } = context.repo; | |
| let prNumber; | |
| let prUserLogin; | |
| if (event === 'pull_request' || event === 'pull_request_review') { | |
| const pr = context.payload.pull_request; | |
| if (!pr) { | |
| core.setFailed('Pull request context not found for event: ' + event); | |
| return; | |
| } | |
| prNumber = pr.number; | |
| prUserLogin = pr.user?.login; | |
| } else if (event === 'workflow_run') { | |
| const workflowRun = context.payload.workflow_run; | |
| const associatedPr = workflowRun?.pull_requests?.[0]; | |
| if (!associatedPr) { | |
| core.info('Workflow run is not associated with a pull request - skipping review enforcement.'); | |
| return; | |
| } | |
| prNumber = associatedPr.number; | |
| const prResponse = await github.rest.pulls.get({ | |
| owner, | |
| repo, | |
| pull_number: prNumber | |
| }); | |
| prUserLogin = prResponse.data.user?.login; | |
| } else { | |
| core.info(`Event '${event}' is not supported by this workflow - skipping review enforcement.`); | |
| return; | |
| } | |
| const isDependabot = prUserLogin === 'dependabot[bot]'; | |
| // Skip for Dependabot on all events | |
| if (isDependabot) { | |
| core.info('Dependabot PR detected - skipping review enforcement.'); | |
| return; | |
| } | |
| core.info('Checking current approval status for pull request #' + prNumber + '...'); | |
| const reviews = await github.rest.pulls.listReviews({ | |
| owner, | |
| repo, | |
| pull_number: prNumber |
| const approvals = reviews.data.filter(r => r.state === 'APPROVED'); | ||
| const hasNonBazApproval = approvals.some( | ||
| r => r.user?.login && | ||
| r.user.login !== 'baz-reviewer' && | ||
| r.user.type === 'User' |
There was a problem hiding this comment.
The approval check counts any review with state APPROVED, even if the same reviewer later requested changes or the approval was dismissed. GitHub review state is effectively “latest review per user”, so this can incorrectly pass. Compute each reviewer’s latest review state (e.g., by sorting by submitted_at and keeping the most recent per user.login) before deciding whether an approval exists.
| const approvals = reviews.data.filter(r => r.state === 'APPROVED'); | |
| const hasNonBazApproval = approvals.some( | |
| r => r.user?.login && | |
| r.user.login !== 'baz-reviewer' && | |
| r.user.type === 'User' | |
| // Determine the latest review per user (GitHub treats review state as latest review per user) | |
| const latestReviewsByUser = new Map(); | |
| for (const r of reviews.data) { | |
| const user = r.user; | |
| const login = user?.login; | |
| if (!login || user.type !== 'User') { | |
| continue; | |
| } | |
| const existing = latestReviewsByUser.get(login); | |
| if ( | |
| !existing || | |
| (r.submitted_at && new Date(r.submitted_at) > new Date(existing.submitted_at)) | |
| ) { | |
| latestReviewsByUser.set(login, r); | |
| } | |
| } | |
| const approvals = Array.from(latestReviewsByUser.values()).filter( | |
| r => r.state === 'APPROVED' | |
| ); | |
| const hasNonBazApproval = approvals.some( | |
| r => r.user?.login && r.user.login !== 'baz-reviewer' |
| const reviews = await github.rest.pulls.listReviews({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: pr.number | ||
| }); | ||
|
|
||
| const approvals = reviews.data.filter(r => r.state === 'APPROVED'); |
There was a problem hiding this comment.
pulls.listReviews is paginated (defaults to 30). For PRs with many reviews, the required approval could be on a later page and be missed. Set per_page: 100 and paginate through all pages (or use Octokit pagination helpers) before evaluating approvals.
| const reviews = await github.rest.pulls.listReviews({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| pull_number: pr.number | |
| }); | |
| const approvals = reviews.data.filter(r => r.state === 'APPROVED'); | |
| const reviews = await github.paginate( | |
| github.rest.pulls.listReviews, | |
| { | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| pull_number: pr.number, | |
| per_page: 100 | |
| } | |
| ); | |
| const approvals = reviews.filter(r => r.state === 'APPROVED'); |
| enforce-review: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| pull-requests: write |
There was a problem hiding this comment.
This job only reads PR data; it doesn’t perform any PR write operations. Consider reducing permissions from pull-requests: write to pull-requests: read here to follow least-privilege.
| pull-requests: write | |
| pull-requests: read |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| const pr = context.payload.pull_request; | ||
| const event = context.eventName; | ||
| const isDependabot = pr.user?.login === 'dependabot[bot]'; | ||
|
|
||
| // Skip for Dependabot on all events | ||
| if (isDependabot) { | ||
| core.info('Dependabot PR detected - skipping review enforcement.'); | ||
| return; | ||
| } | ||
|
|
||
| // Only enforce review requirement on pull_request_review events | ||
| if (event === 'pull_request') { | ||
| core.info('PR opened/synchronized - waiting for review, not enforcing yet.'); | ||
| return; | ||
| } | ||
|
|
||
| // From here on, we know it's a pull_request_review event (submitted or dismissed): enforce rule | ||
| core.info('Review event detected - checking current approval status...'); | ||
|
|
||
| const reviews = await github.rest.pulls.listReviews({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: pr.number | ||
| }); |
There was a problem hiding this comment.
context.payload.pull_request is undefined on workflow_run events, but the script reads pr.user?.login before checking the event type, so the job will throw before reaching the guard whenever the workflow_run trigger fires; can we guard against missing pull_request (or skip workflow_run) before dereferencing pr?
Finding type: Logical Bugs
Prompt for AI Agents:
In .github/workflows/review-enforcement.yml around lines 22 to 45, the script assigns
const pr = context.payload.pull_request and then immediately reads pr.user?.login, but
on workflow_run events payload.pull_request is undefined causing a crash. Change the
logic to first check the event type and/or the existence of context.payload.pull_request
before dereferencing pr: either move the event/type check above the isDependabot check
or add a guard like if (!context.payload.pull_request) { core.info('No pull_request in
payload - skipping'); return; } so workflow_run runs don’t throw when pull_request is
missing.
User description
Summary
review-enforcement.ymlworkflow from the API repoTest plan
🤖 Generated with Claude Code
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Implements a new GitHub Action workflow to enforce pull request review policies and automate reviewer assignment for failing Dependabot builds. Ensures that all non-Dependabot PRs receive at least one approval from a human reviewer who is not
baz-reviewer.baz-reviewer) for standard pull requests.Modified files (1)
Checks (1)
Latest Contributors(0)
rungalileo/productteam when a Dependabot PR fails its CI tests.Modified files (1)
Checks (1)
Latest Contributors(0)