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 10 commits into
Conversation
github-actions
Bot
added
approved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe bot now adds all requested individual reviewers as assignees, removes reviewers after submitted reviews, and uses review artifacts to connect the capture and removal workflows. Tests cover routing, no-op cases, uncapped additions, and permission errors. ChangesReviewer-as-Assignee Bot: Add/Remove Flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequestReview
participant CapturePRReview
participant OnReviewWorkflow
participant ReviewerAssigneeBot
participant GitHubREST
PullRequestReview->>CapturePRReview: submitted review event
CapturePRReview->>OnReviewWorkflow: upload reviewer and PR metadata
OnReviewWorkflow->>ReviewerAssigneeBot: invoke removal flow
ReviewerAssigneeBot->>GitHubREST: fetch live pull request state
ReviewerAssigneeBot->>GitHubREST: remove assigned reviewer
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ 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
exploreriii
removed
the
status: update branch
|
@exploreriii Thanks for the review! Addressing the permissions concerns:
The end-to-end add + remove flow was verified on a fork PR here: Mounil2005#41 |
|
Note the real test will have to be a fork of a given repository, creating a pull request to your repository. So if the repo is called alice.python, the pr will need to be made from alice.python.fork. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: af8779b2-0c78-4cbf-b7da-2adee557ad51
📒 Files selected for processing (4)
.github/scripts/__tests__/jest/bot-pr-add-reviewers-as-assignees.test.js.github/scripts/bot-pr-add-reviewers-as-assignees.js.github/workflows/capture-pr-review.yml.github/workflows/on-review.yml
9c1ad79 to
1de64a6
Compare
aceppaluni
added
the
status: update branch
|
@Mounil2005 can you please update the branch |
3d4dc5e to
dbe0e46
Compare
- 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>
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>
pull_request_review events receive a read-only GITHUB_TOKEN for PRs from forked repositories, so removeAssignees would silently 403 for every real contribution. The workflow_run pattern is the standard GitHub-recommended fix: a lightweight capture workflow fires on pull_request_review (read-only token is sufficient — it only writes an artifact), and the main workflow's workflow_run job picks it up with a full write token regardless of fork status. Changes: - Add capture-pr-review.yml: triggered by pull_request_review, writes reviewer login and PR number to a JSON artifact, uploads with a 1-day retention so workflow_run can consume it - on-review.yml: remove pull_request_review trigger; add workflow_run trigger and a new remove-reviewer-from-assignees job that downloads the artifact, checks out base-branch code, and calls the named export directly; add actions: read permission for artifact download; tighten add job if-condition to exclude workflow_run events - bot-pr-add-reviewers-as-assignees.js: accept optional explicit reviewer/prNumber params in removeReviewerFromAssignees so it can be called from the workflow_run job without a pull_request_review context; export the function as a named export; drop pull_request_review routing from the entry point (now dead code) - Tests: call removeReviewerFromAssignees via named export with explicit params matching the new production path; add missing/invalid param guard tests; add routing test for the now-unhandled pull_request_review event Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
CodeQL flagged the download-artifact + require() pattern as artifact poisoning: data written by an external user in the capture workflow flows into a write-token context without validation. Validate reviewer login against the GitHub login charset (alphanumeric and hyphens) and confirm pr_number is a positive integer before passing either value to the removal function. Fails the step with a clear message if contents are unexpected. Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
…ror with explicit pre-check - capture-pr-review.yml: validate reviewer login and PR number before writing the artifact; throw with a clear message if the payload is malformed so the capture job fails visibly instead of publishing a bad artifact downstream - on-review.yml: remove continue-on-error on the download step; add an explicit artifact pre-check step that lists artifacts for the triggering run and sets an output; all subsequent steps gate on that output so a missing artifact (bot actor skip) is logged as a notice and the job exits cleanly, while an unexpected download failure now surfaces as a proper job failure Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
…ts on remove job - Move checkout before artifact download so project files are established in the workspace before any untrusted artifact data lands, preventing potential file overwrites - Add path: artifact to download-artifact so the JSON file is scoped to a subdirectory instead of the workspace root; update readFileSync path accordingly - Add bot-skip condition to the remove job using workflow_run.triggering_actor.login so the job exits early for bot actors even if an artifact somehow exists Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
dbe0e46 to
0b0d4af
Compare
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
Hi @Mounil2005
Thanks for splitting this up, I think its quite safe now and should work.
Only small suggestions which I would like to hold you to as this one is an important workflow
| await script({ github, context }); | ||
|
|
||
| remove-reviewer-from-assignees: | ||
| name: Remove Reviewer from Assignees |
There was a problem hiding this comment.
We can give this workflow its own set of permissions so it doesn't inherit any top level ones, in case they change
| contents: read | ||
| pull-requests: write | ||
| issues: write | ||
| actions: read |
There was a problem hiding this comment.
i think only the remove-reviewer requires actions read, so you can remove it here
| const reviewer = explicitReviewer ?? context.payload?.review?.user?.login; | ||
| const prNumber = explicitPrNumber ?? context.payload?.pull_request?.number; | ||
|
|
||
| if (!reviewer || !Number.isInteger(prNumber) || prNumber <= 0) { |
There was a problem hiding this comment.
we can tighten this up to check it is a string and in the regex pattern matching github logins
i.e. at the top add
const VALID_LOGIN_REGEX ..
| @@ -5,9 +5,12 @@ | |||
|
|
|||
| describe('Bot: Add Reviewers as Assignees', () => { | |||
There was a problem hiding this comment.
now that you'd have a regex parse for the reviewer, you can test it will skip an invalid github name for the reviewer
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.)