feat: make reviewer-to-assignee bot dynamic with removal support#2391
feat: make reviewer-to-assignee bot dynamic with removal support#2391Mounil2005 wants to merge 5 commits into
Conversation
github-actions
Bot
added
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (2)
WalkthroughThe bot now adds all requested individual reviewers as assignees, removes reviewers after submitted reviews, and updates workflow routing, checkout behavior, and test coverage for both flows. ChangesReviewer-as-Assignee Bot: Add/Remove Flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubWorkflow
participant ReviewerAssigneeBot
participant GitHubREST
GitHubWorkflow->>ReviewerAssigneeBot: review_requested or workflow_dispatch
ReviewerAssigneeBot->>GitHubREST: addAssignees for requested reviewers
GitHubWorkflow->>ReviewerAssigneeBot: submitted review
ReviewerAssigneeBot->>GitHubREST: get live pull request state
ReviewerAssigneeBot->>GitHubREST: removeAssignees for assigned reviewer
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f7400ed-ff5b-474b-b35b-7c101d490c33
📒 Files selected for processing (3)
.github/scripts/__tests__/jest/bot-pr-add-reviewers-as-assignees.test.js.github/scripts/bot-pr-add-reviewers-as-assignees.js.github/workflows/on-review.yml
github-actions
Bot
added
open to community review
|
@Mounil2005, thanks for the PR! The overall changes look good. Could you also add a link to your testing on your fork for these changes? |
aceppaluni
added
status: update branch
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/on-review.yml (1)
7-9: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winDon't execute checked-out PR code on the new
pull_request_reviewpath.Adding this trigger makes the later
actions/checkout+ localrequire('./.github/scripts/bot-pr-add-reviewers-as-assignees.js')run against the PR’s checked-out contents on review submissions. For forked PRs, that lets untrusted contributors influence the JS this workflow executes, which violates the fork-safety requirement.Suggested fix
- name: Checkout repository uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: + ref: ${{ github.event_name == 'pull_request_review' && github.event.pull_request.base.sha || github.sha }} persist-credentials: falseAs per path instructions, workflows must behave safely when executed from forks.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 58dfad66-38da-4641-97d3-2ef5995ee9e8
📒 Files selected for processing (1)
.github/workflows/on-review.yml
|
Hello, this is the OfficeHourBot. This is a reminder that the Hiero Python SDK Office Hours will begin in approximately 2 hours and 57 minutes (14:00 UTC). This session provides an opportunity to ask questions regarding this Pull Request. Details:
Disclaimer: This is an automated reminder. Please verify the schedule here for any changes. From, |
Link to the testing! Sorry for the delay! |
|
Once everything looks good, pls do let me know, i will then reset my main and rebase |
|
@manishdait @aceppaluni @exploreriii Can you pls review this and let me know if this is alright? |
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
Is it intentional that a reviewer who leaves only a comment (without approving or requesting changes) is considered "done"?
Yes, intentional. The rationale is that any review submission whether approval, changes requested, or comment signals the reviewer has actively engaged with the PR. The assignee field is used to track who still needs to act; once they've submitted any review, the ball is back in the author's court regardless of the review type. |
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
Hi @Mounil2005
Most of the code looks pretty good but have another check through the logic of the permissions
| add-reviewers-as-assignees: | ||
| name: Add Reviewers as Assignees | ||
| runs-on: hl-sdk-py-lin-md | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
needs to use the custom runner
| } | ||
|
|
||
| /** | ||
| * Removes a reviewer from PR assignees when they submit their review. |
There was a problem hiding this comment.
Because you are writing reviewers, this PR may not work.
"The GITHUB_TOKEN has read-only permissions in pull requests from forked repositories"
https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows
You can test to confirm, and if it fails you'd have to try another way / archive the issue
| */ | ||
|
|
||
| const { createLogger, MAX_ASSIGNEES, BOT_NAME_ASSIGNEES } = require('./shared/helpers/reviewers-assignee-index.js'); | ||
| const { createLogger, BOT_NAME_ASSIGNEES } = require('./shared/helpers/reviewers-assignee-index.js'); |
There was a problem hiding this comment.
Since you removed MAX_ASSIGNEES, that will be dead code in the repo as nothing else depends on it, i think
|
|
||
| concurrency: | ||
| group: reviewer-assignee-${{ github.event.pull_request.number || inputs.pr_number || github.run_id }} | ||
| group: >- |
There was a problem hiding this comment.
an add and a remove for the same PR can still run concurrently
| runs-on: hl-sdk-py-lin-md | ||
| runs-on: ubuntu-latest | ||
|
|
||
| concurrency: |
exploreriii
removed
the
status: update branch
exploreriii
removed
queue:junior-committer
- Add pull_request_review: submitted trigger to on-review.yml so the workflow fires when a reviewer submits their feedback - Split the bot into addReviewersAsAssignees / removeReviewerFromAssignees functions, routing by eventName + action - Remove the MAX_ASSIGNEES=2 cap so all requested reviewers are assigned - Mirror the existing 403 guard on the new removal path - Update tests: drop the cap assertion, add removal flow coverage (happy path, no-op, 403, rethrow) and workflow_dispatch routing check Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
…iewer
- In removeReviewerFromAssignees, replace the stale event-payload
assignee snapshot with a live pulls.get call so a concurrent
review_requested run that adds the assignee after the event fires
does not cause the reviewer to remain assigned permanently
- Give pull_request_review runs a reviewer-scoped concurrency key
(reviewer-removal-{PR}-{login}) so concurrent review submissions
from different reviewers are not cancelled by each other
- Update removal-flow tests to populate state.currentPrData.assignees
so the mock pulls.get returns the correct live assignee list; fix
error-path mocks to return { data: { assignees: [...] } }
Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
Give pull_request_review runs a per-reviewer concurrency slot
(reviewer-removal-{PR}-{login}) so simultaneous review submissions
from multiple reviewers are not cancelled by each other.
Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
- Remove MAX_ASSIGNEES from constants.js and reviewers-assignee-index.js;
it was dead code since nothing reads it after the cap was removed
- Unify the concurrency group to reviewer-assignee-{PR} for all event
types so an add and remove for the same PR cannot race
- Skip the job entirely when the actor is a bot (contains '[bot]') to
avoid triggering on dependabot or other automated review requests
Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
Mounil2005
force-pushed
the
bot/dynamic-reviewer-assignees
branch
from
affff66 to
b3f2129
Compare
For pull_request_review events github.sha resolves to the PR head commit (i.e. the fork's branch), so the default actions/checkout would pull untrusted contributor code and the subsequent require() call would execute the fork's copy of the bot script. Explicitly set ref to github.event.pull_request.base.sha so the checkout always lands on the verified base-branch code. Falls back to github.sha for workflow_dispatch where pull_request is absent. Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
github-actions
Bot
added
open to community review
There was a problem hiding this comment.
Pull request overview
This PR updates the “reviewer-to-assignee” GitHub Action bot to dynamically manage assignees over a PR’s lifecycle: requested reviewers are added as assignees, and reviewers are removed from assignees once they submit a review. It also removes the previous artificial cap on the number of assignees to better match the workflow described in issue #2349.
Changes:
- Add
pull_request_review: submittedas a workflow trigger and update concurrency/checkout behavior for safer, more consistent execution. - Split bot behavior into explicit add/remove flows and route based on
eventName+action. - Remove the
MAX_ASSIGNEEScap and update unit tests to cover removal scenarios and new routing.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/on-review.yml | Adds pull_request_review trigger; adjusts concurrency and checkout ref selection. |
| .github/scripts/shared/helpers/reviewers-assignee-index.js | Removes export of MAX_ASSIGNEES now that the cap is dropped. |
| .github/scripts/shared/helpers/constants.js | Removes MAX_ASSIGNEES constant. |
| .github/scripts/bot-pr-add-reviewers-as-assignees.js | Refactors into add/remove flows and routes per triggering event; removes assignee cap. |
| .github/scripts/tests/jest/bot-pr-add-reviewers-as-assignees.test.js | Updates mocks and assertions; adds coverage for removal flow and workflow_dispatch routing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| add-reviewers-as-assignees: | ||
| name: Add Reviewers as Assignees | ||
| runs-on: hl-sdk-py-lin-md | ||
| if: "!contains(github.actor, '[bot]')" |
Description:
Make the reviewer-to-assignee bot dynamic: reviewers are added when requested and removed when they submit their review, with no artificial cap on assignee count.
Related issue(s):
Fixes #2349
Notes for reviewer:
The removal fires on any review state (approved, changes_requested, commented) — any submission signals the reviewer has completed their work.
Checklist
[x] Documented (Code comments, README, etc.)
[x]Tested (unit, integration, etc.)