fix: use workflow_run to remove assignees with write token on fork PRs

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>

Author: Mounil2005

.github/scripts/__tests__/jest/bot-pr-add-reviewers-as-assignees.test.js

@@ -5,9 +5,12 @@
 
 describe('Bot: Add Reviewers as Assignees', () => {
   let handler;
+  let removeReviewerFromAssignees;
 
   beforeAll(() => {
-    handler = require('../../bot-pr-add-reviewers-as-assignees.js');
+    const mod = require('../../bot-pr-add-reviewers-as-assignees.js');
+    handler = mod;
+    removeReviewerFromAssignees = mod.removeReviewerFromAssignees;
   });
 
   beforeEach(() => {
@@ -36,20 +39,8 @@ describe('Bot: Add Reviewers as Assignees', () => {
     }
   });
 
-  const createMockReviewContext = ({ reviewerLogin, assignees = [] } = {}) => ({
-    repo: { owner: 'hiero-ledger', repo: 'hiero-sdk-python' },
-    eventName: 'pull_request_review',
-    payload: {
-      action: 'submitted',
-      review: {
-        user: { login: reviewerLogin }
-      },
-      pull_request: {
-        number: 123,
-        assignees
-      }
-    }
-  });
+  // Minimal context for remove flow: only repo is required when explicit params are passed
+  const minimalContext = { repo: { owner: 'hiero-ledger', repo: 'hiero-sdk-python' } };
 
   const createMockGithub = (state) => ({
     rest: {
@@ -247,56 +238,81 @@ describe('Bot: Add Reviewers as Assignees', () => {
     await expect(handler({ github: errorMock, context: ctx })).rejects.toHaveProperty('status', 500);
   });
 
-  // ─── Remove flow ─────────────────────────────────────────────────────────────
+  // ─── Remove flow (named export — called from workflow_run job) ────────────────
 
-  test('removes reviewer from assignees when they submit a review', async () => {
+  test('removes reviewer from assignees when called with explicit params', async () => {
     const state = createTestState();
     state.currentPrData = { assignees: [{ login: 'alice' }, { login: 'bob' }] };
-    const ctx = createMockReviewContext({
-      reviewerLogin: 'alice',
-      assignees: [{ login: 'alice' }, { login: 'bob' }]
-    });
 
-    await handler({ github: createMockGithub(state), context: ctx });
+    await removeReviewerFromAssignees({
+      github: createMockGithub(state),
+      context: minimalContext,
+      reviewer: 'alice',
+      prNumber: 123
+    });
 
     expect(state.removeAssigneesCalls).toHaveLength(1);
     expect(state.removeAssigneesCalls[0].assignees).toEqual(['alice']);
     expect(state.addAssigneesCalls).toHaveLength(0);
   });
 
-  test('does not call removeAssignees when reviewer is not an assignee', async () => {
+  test('does not remove when reviewer is not an assignee', async () => {
     const state = createTestState();
     state.currentPrData = { assignees: [{ login: 'alice' }] };
-    const ctx = createMockReviewContext({
-      reviewerLogin: 'carol',
-      assignees: [{ login: 'alice' }]
-    });
 
-    await handler({ github: createMockGithub(state), context: ctx });
+    await removeReviewerFromAssignees({
+      github: createMockGithub(state),
+      context: minimalContext,
+      reviewer: 'carol',
+      prNumber: 123
+    });
 
     expect(state.removeAssigneesCalls).toHaveLength(0);
-    expect(state.addAssigneesCalls).toHaveLength(0);
   });
 
-  test('review submitted by someone who was never a reviewer is a no-op', async () => {
+  test('is a no-op when reviewer was never an assignee', async () => {
     const state = createTestState();
     state.currentPrData = { assignees: [] };
-    const ctx = createMockReviewContext({
-      reviewerLogin: 'outsider',
-      assignees: []
+
+    await removeReviewerFromAssignees({
+      github: createMockGithub(state),
+      context: minimalContext,
+      reviewer: 'outsider',
+      prNumber: 123
     });
 
-    await handler({ github: createMockGithub(state), context: ctx });
+    expect(state.removeAssigneesCalls).toHaveLength(0);
+  });
+
+  test('skips when reviewer is missing', async () => {
+    const state = createTestState();
+
+    await removeReviewerFromAssignees({
+      github: createMockGithub(state),
+      context: minimalContext,
+      reviewer: undefined,
+      prNumber: 123
+    });
 
     expect(state.removeAssigneesCalls).toHaveLength(0);
+    expect(state.pullsGetCalls).toBe(0);
   });
 
-  test('gracefully handles 403 permission errors on remove', async () => {
-    const ctx = createMockReviewContext({
-      reviewerLogin: 'alice',
-      assignees: [{ login: 'alice' }]
+  test('skips when prNumber is invalid', async () => {
+    const state = createTestState();
+
+    await removeReviewerFromAssignees({
+      github: createMockGithub(state),
+      context: minimalContext,
+      reviewer: 'alice',
+      prNumber: 0
     });
 
+    expect(state.removeAssigneesCalls).toHaveLength(0);
+    expect(state.pullsGetCalls).toBe(0);
+  });
+
+  test('gracefully handles 403 permission errors on remove', async () => {
     const errorMock = {
       rest: {
         pulls: { get: async () => ({ data: { assignees: [{ login: 'alice' }] } }) },
@@ -311,15 +327,12 @@ describe('Bot: Add Reviewers as Assignees', () => {
       }
     };
 
-    await expect(handler({ github: errorMock, context: ctx })).resolves.not.toThrow();
+    await expect(
+      removeReviewerFromAssignees({ github: errorMock, context: minimalContext, reviewer: 'alice', prNumber: 123 })
+    ).resolves.not.toThrow();
   });
 
   test('rethrows non-403 errors on remove', async () => {
-    const ctx = createMockReviewContext({
-      reviewerLogin: 'alice',
-      assignees: [{ login: 'alice' }]
-    });
-
     const errorMock = {
       rest: {
         pulls: { get: async () => ({ data: { assignees: [{ login: 'alice' }] } }) },
@@ -334,6 +347,24 @@ describe('Bot: Add Reviewers as Assignees', () => {
       }
     };
 
-    await expect(handler({ github: errorMock, context: ctx })).rejects.toHaveProperty('status', 500);
+    await expect(
+      removeReviewerFromAssignees({ github: errorMock, context: minimalContext, reviewer: 'alice', prNumber: 123 })
+    ).rejects.toHaveProperty('status', 500);
+  });
+
+  // ─── Routing ─────────────────────────────────────────────────────────────────
+
+  test('unhandled event logs warning and does nothing', async () => {
+    const state = createTestState();
+    const ctx = {
+      repo: { owner: 'hiero-ledger', repo: 'hiero-sdk-python' },
+      eventName: 'pull_request_review',
+      payload: { action: 'submitted' }
+    };
+
+    await handler({ github: createMockGithub(state), context: ctx });
+
+    expect(state.addAssigneesCalls).toHaveLength(0);
+    expect(state.removeAssigneesCalls).toHaveLength(0);
   });
 });

.github/scripts/bot-pr-add-reviewers-as-assignees.js

@@ -4,8 +4,10 @@
  * @fileoverview
  * Manages requested individual reviewers as PR assignees.
  *
- * - On `review_requested`: adds the reviewer as an assignee (no cap).
- * - On `pull_request_review` submitted: removes the reviewer from assignees.
+ * - On `review_requested` (pull_request_target): adds the reviewer as an assignee (no cap).
+ * - On `workflow_run` (triggered by Bot - Capture PR Review): removes the reviewer from
+ *   assignees using a write-token that works for fork PRs. The reviewer login and PR number
+ *   are read from the artifact written by the capture workflow.
  * - Team reviewers are intentionally ignored (only individual users are assigned).
  */
 
@@ -125,18 +127,22 @@ async function addReviewersAsAssignees({ github, context }) {
 }
 
 /**
- * Removes a reviewer from PR assignees when they submit their review.
- * Any review state (approved, changes_requested, commented) triggers removal.
+ * Removes a reviewer from PR assignees after they submit their review.
+ * Called from the workflow_run removal job via named export, passing reviewer
+ * login and PR number read from the capture workflow's artifact.
+ *
+ * Explicit params take priority; falls back to context.payload for direct calls.
  *
  * @param {Object} params
  * @param {Object} params.github - GitHub Octokit client instance
  * @param {Object} params.context - GitHub Actions context object
+ * @param {string} [params.reviewer] - Reviewer login (overrides context payload)
+ * @param {number} [params.prNumber] - PR number (overrides context payload)
  */
-async function removeReviewerFromAssignees({ github, context }) {
+async function removeReviewerFromAssignees({ github, context, reviewer: explicitReviewer, prNumber: explicitPrNumber }) {
   try {
-    const reviewer = context.payload.review?.user?.login;
-    const pr = context.payload.pull_request;
-    const prNumber = pr?.number;
+    const reviewer = explicitReviewer ?? context.payload?.review?.user?.login;
+    const prNumber = explicitPrNumber ?? context.payload?.pull_request?.number;
 
     if (!reviewer || !Number.isInteger(prNumber) || prNumber <= 0) {
       logger.warn('Missing reviewer login or PR number. Skipping removal.');
@@ -146,8 +152,7 @@ async function removeReviewerFromAssignees({ github, context }) {
     const owner = context.repo.owner;
     const repo = context.repo.repo;
 
-    // Fetch live PR data rather than relying on the event payload snapshot,
-    // which may be stale if review_requested ran concurrently and added the assignee after this event fired.
+    // Fetch live PR data to avoid acting on a stale event payload snapshot.
     const livePr = (await github.rest.pulls.get({ owner, repo, pull_number: prNumber })).data;
     const currentAssignees = (livePr.assignees || []).map(a => a.login);
 
@@ -178,7 +183,8 @@ async function removeReviewerFromAssignees({ github, context }) {
 }
 
 /**
- * Entry point. Routes to add or remove flow based on the triggering event.
+ * Entry point for the add flow (pull_request_target + workflow_dispatch).
+ * The remove flow is invoked directly via the named export from the workflow_run job.
  *
  * @param {Object} params
  * @param {Object} params.github - GitHub Octokit client instance
@@ -189,9 +195,7 @@ module.exports = async ({ github, context }) => {
   const { eventName } = context;
   const action = context.payload.action;
 
-  if (eventName === 'pull_request_review' && action === 'submitted') {
-    await removeReviewerFromAssignees({ github, context });
-  } else if (
+  if (
     (eventName === 'pull_request_target' && action === 'review_requested') ||
     eventName === 'workflow_dispatch'
   ) {
@@ -200,3 +204,5 @@ module.exports = async ({ github, context }) => {
     logger.warn(`Unhandled event: ${eventName} / ${action}. Skipping.`);
   }
 };
+
+module.exports.removeReviewerFromAssignees = removeReviewerFromAssignees;

.github/workflows/capture-pr-review.yml

@@ -0,0 +1,38 @@
+name: Bot - Capture PR Review
+
+on:
+  pull_request_review:
+    types:
+      - submitted
+
+permissions:
+  contents: read
+
+jobs:
+  capture:
+    name: Capture Review Metadata
+    runs-on: hl-sdk-py-lin-md
+    if: "!contains(github.actor, '[bot]')"
+
+    steps:
+      - name: Harden Runner
+        uses: step-security/harden-runner@bf7454d06d71f1098171f2acdf0cd4708d7b5920 # v2.20.0
+        with:
+          egress-policy: audit
+
+      - name: Write review metadata
+        uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
+        with:
+          script: |
+            const fs = require('fs');
+            fs.mkdirSync('review-data', { recursive: true });
+            const reviewer = context.payload.review?.user?.login ?? null;
+            const pr_number = context.payload.pull_request?.number ?? null;
+            fs.writeFileSync('review-data/review.json', JSON.stringify({ reviewer, pr_number }));
+
+      - name: Upload review metadata
+        uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
+        with:
+          name: review-data-${{ github.run_id }}
+          path: review-data/review.json
+          retention-days: 1

.github/workflows/on-review.yml

@@ -4,9 +4,11 @@ on:
   pull_request_target:
     types:
       - review_requested
-  pull_request_review:
+  workflow_run:
+    workflows:
+      - "Bot - Capture PR Review"
     types:
-      - submitted
+      - completed
   workflow_dispatch:
     inputs:
       pr_number:
@@ -18,12 +20,18 @@ permissions:
   contents: read
   pull-requests: write
   issues: write
+  actions: read
 
 jobs:
   add-reviewers-as-assignees:
     name: Add Reviewers as Assignees
     runs-on: hl-sdk-py-lin-md
-    if: "!contains(github.actor, '[bot]')"
+    if: |
+      !contains(github.actor, '[bot]') &&
+      (
+        (github.event_name == 'pull_request_target' && github.event.action == 'review_requested') ||
+        github.event_name == 'workflow_dispatch'
+      )
 
     concurrency:
       group: reviewer-assignee-${{ github.event.pull_request.number || inputs.pr_number || github.run_id }}
@@ -47,3 +55,41 @@ jobs:
           script: |
             const script = require('./.github/scripts/bot-pr-add-reviewers-as-assignees.js');
             await script({ github, context });
+
+  remove-reviewer-from-assignees:
+    name: Remove Reviewer from Assignees
+    runs-on: hl-sdk-py-lin-md
+    if: |
+      github.event_name == 'workflow_run' &&
+      github.event.workflow_run.conclusion == 'success'
+
+    steps:
+      - name: Harden Runner
+        uses: step-security/harden-runner@bf7454d06d71f1098171f2acdf0cd4708d7b5920 # v2.20.0
+        with:
+          egress-policy: audit
+
+      - name: Download review metadata
+        id: download
+        uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1
+        with:
+          name: review-data-${{ github.event.workflow_run.id }}
+          run-id: ${{ github.event.workflow_run.id }}
+          github-token: ${{ secrets.GITHUB_TOKEN }}
+        continue-on-error: true
+
+      - name: Checkout repository
+        if: steps.download.outcome == 'success'
+        uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
+        with:
+          persist-credentials: false
+
+      - name: Run Remove Reviewer from Assignees
+        if: steps.download.outcome == 'success'
+        uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
+        with:
+          script: |
+            const fs = require('fs');
+            const { reviewer, pr_number: prNumber } = JSON.parse(fs.readFileSync('review.json', 'utf8'));
+            const { removeReviewerFromAssignees } = require('./.github/scripts/bot-pr-add-reviewers-as-assignees.js');
+            await removeReviewerFromAssignees({ github, context, reviewer, prNumber });