diff --git a/.github/scripts/__tests__/jest/bot-pr-add-reviewers-as-assignees.test.js b/.github/scripts/__tests__/jest/bot-pr-add-reviewers-as-assignees.test.js index 90daab6ac..abce8b350 100644 --- a/.github/scripts/__tests__/jest/bot-pr-add-reviewers-as-assignees.test.js +++ b/.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(() => { @@ -16,6 +19,7 @@ describe('Bot: Add Reviewers as Assignees', () => { const createTestState = () => ({ addAssigneesCalls: [], + removeAssigneesCalls: [], pullsGetCalls: 0, currentPrData: null, }); @@ -24,6 +28,7 @@ describe('Bot: Add Reviewers as Assignees', () => { repo: { owner: 'hiero-ledger', repo: 'hiero-sdk-python' }, eventName: 'pull_request_target', payload: { + action: 'review_requested', pull_request: { number: 123, requested_reviewers: [], @@ -34,6 +39,9 @@ describe('Bot: Add Reviewers as 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: { pulls: { @@ -53,11 +61,17 @@ describe('Bot: Add Reviewers as Assignees', () => { addAssignees: async (params) => { state.addAssigneesCalls.push(params); return { data: {} }; + }, + removeAssignees: async (params) => { + state.removeAssigneesCalls.push(params); + return { data: {} }; } } } }); + // ─── Add flow ──────────────────────────────────────────────────────────────── + test('adds individual reviewers correctly as assignees', async () => { const state = createTestState(); state.currentPrData = { @@ -76,6 +90,23 @@ describe('Bot: Add Reviewers as Assignees', () => { expect(call.assignees.sort()).toEqual(['alice', 'bob']); }); + test('adds all requested reviewers as assignees without cap', async () => { + const state = createTestState(); + state.currentPrData = { + requested_reviewers: [{ login: 'u1' }, { login: 'u2' }, { login: 'u3' }], + assignees: [] + }; + + const ctx = createMockContext({ + requested_reviewers: [{ login: 'u1' }, { login: 'u2' }, { login: 'u3' }] + }); + + await handler({ github: createMockGithub(state), context: ctx }); + + expect(state.addAssigneesCalls).toHaveLength(1); + expect(state.addAssigneesCalls[0].assignees.sort()).toEqual(['u1', 'u2', 'u3']); + }); + test('ignores team reviewers', async () => { const state = createTestState(); state.currentPrData = { @@ -112,23 +143,6 @@ describe('Bot: Add Reviewers as Assignees', () => { expect(state.addAssigneesCalls).toHaveLength(0); }); - test('respects MAX_ASSIGNEES = 2 cap', async () => { - const state = createTestState(); - state.currentPrData = { - requested_reviewers: [{ login: 'u1' }, { login: 'u2' }, { login: 'u3' }], - assignees: [] - }; - - const ctx = createMockContext({ - requested_reviewers: [{ login: 'u1' }, { login: 'u2' }, { login: 'u3' }] - }); - - await handler({ github: createMockGithub(state), context: ctx }); - - expect(state.addAssigneesCalls).toHaveLength(1); - expect(state.addAssigneesCalls[0].assignees).toHaveLength(2); - }); - test('does nothing when no reviewers are requested', async () => { const state = createTestState(); const ctx = createMockContext({ requested_reviewers: [] }); @@ -150,7 +164,7 @@ describe('Bot: Add Reviewers as Assignees', () => { expect(state.addAssigneesCalls).toHaveLength(0); }); - test('supports workflow_dispatch with pr_number input', async () => { + test('supports workflow_dispatch with pr_number input and routes to add flow', async () => { const state = createTestState(); state.currentPrData = { requested_reviewers: [{ login: 'eve' }], assignees: [] }; @@ -164,6 +178,7 @@ describe('Bot: Add Reviewers as Assignees', () => { expect(state.addAssigneesCalls).toHaveLength(1); expect(state.addAssigneesCalls[0].issue_number).toBe(128); + expect(state.removeAssigneesCalls).toHaveLength(0); }); test('handles invalid pr_number in workflow_dispatch', async () => { @@ -183,7 +198,7 @@ describe('Bot: Add Reviewers as Assignees', () => { } }); - test('gracefully handles 403 permission errors', async () => { + test('gracefully handles 403 permission errors on add', async () => { const ctx = createMockContext({ requested_reviewers: [{ login: 'x' }] }); const errorMock = { @@ -194,7 +209,8 @@ describe('Bot: Add Reviewers as Assignees', () => { const err = new Error('Forbidden'); err.status = 403; throw err; - } + }, + removeAssignees: async () => {} } } }; @@ -202,7 +218,7 @@ describe('Bot: Add Reviewers as Assignees', () => { await expect(handler({ github: errorMock, context: ctx })).resolves.not.toThrow(); }); - test('rethrows non-403 errors', async () => { + test('rethrows non-403 errors on add', async () => { const ctx = createMockContext({ requested_reviewers: [{ login: 'x' }] }); const errorMock = { @@ -213,11 +229,142 @@ describe('Bot: Add Reviewers as Assignees', () => { const err = new Error('Internal Server Error'); err.status = 500; throw err; - } + }, + removeAssignees: async () => {} } } }; await expect(handler({ github: errorMock, context: ctx })).rejects.toHaveProperty('status', 500); }); + + // ─── Remove flow (named export — called from workflow_run job) ──────────────── + + test('removes reviewer from assignees when called with explicit params', async () => { + const state = createTestState(); + state.currentPrData = { assignees: [{ login: 'alice' }, { login: 'bob' }] }; + + 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 remove when reviewer is not an assignee', async () => { + const state = createTestState(); + state.currentPrData = { assignees: [{ login: 'alice' }] }; + + await removeReviewerFromAssignees({ + github: createMockGithub(state), + context: minimalContext, + reviewer: 'carol', + prNumber: 123 + }); + + expect(state.removeAssigneesCalls).toHaveLength(0); + }); + + test('is a no-op when reviewer was never an assignee', async () => { + const state = createTestState(); + state.currentPrData = { assignees: [] }; + + await removeReviewerFromAssignees({ + github: createMockGithub(state), + context: minimalContext, + reviewer: 'outsider', + prNumber: 123 + }); + + 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('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' }] } }) }, + issues: { + addAssignees: async () => {}, + removeAssignees: async () => { + const err = new Error('Forbidden'); + err.status = 403; + throw err; + } + } + } + }; + + await expect( + removeReviewerFromAssignees({ github: errorMock, context: minimalContext, reviewer: 'alice', prNumber: 123 }) + ).resolves.not.toThrow(); + }); + + test('rethrows non-403 errors on remove', async () => { + const errorMock = { + rest: { + pulls: { get: async () => ({ data: { assignees: [{ login: 'alice' }] } }) }, + issues: { + addAssignees: async () => {}, + removeAssignees: async () => { + const err = new Error('Internal Server Error'); + err.status = 500; + throw err; + } + } + } + }; + + 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); + }); }); diff --git a/.github/scripts/bot-pr-add-reviewers-as-assignees.js b/.github/scripts/bot-pr-add-reviewers-as-assignees.js index feb819e2f..172e14540 100644 --- a/.github/scripts/bot-pr-add-reviewers-as-assignees.js +++ b/.github/scripts/bot-pr-add-reviewers-as-assignees.js @@ -2,14 +2,16 @@ /** * @fileoverview - * Automatically adds requested individual reviewers as assignees on Pull Requests. + * Manages requested individual reviewers as PR assignees. * - * This is part of the generic "on-review" infrastructure. - * Team reviewers are intentionally ignored (only individual users are assigned). - * Caps the number of assignees at MAX_ASSIGNEES (default: 2). + * - 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). */ -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'); const logger = createLogger(BOT_NAME_ASSIGNEES); @@ -59,37 +61,13 @@ function getUsersToAssign(requestedReviewers, currentAssignees) { } /** - * Logs a warning if some reviewers were dropped due to the assignee cap. - * - * @param {Set} usersToAssign - */ -function logAssigneeCapWarning(usersToAssign, maxToAdd) { - if (usersToAssign.size > maxToAdd) { - const dropped = Array.from(usersToAssign).slice(maxToAdd); - logger.warn(`Assignee cap (${MAX_ASSIGNEES}) reached. Dropping: ${dropped.join(', ')}`); - } -} - -/** - * Main handler that adds requested reviewers as assignees on a PR. - * - * Triggered by: - * - `pull_request_target: review_requested` - * - `workflow_dispatch` (for manual testing) - * - * Behavior: - * - Only processes individual reviewers (`requested_reviewers`) - * - Ignores team reviewers (`requested_teams`) - * - Skips users who are already assignees - * - Caps at `MAX_ASSIGNEES` (default: 2) - * - Logs a warning when reviewers are dropped due to the cap + * Adds requested reviewers as PR assignees (no cap on count). * * @param {Object} params * @param {Object} params.github - GitHub Octokit client instance * @param {Object} params.context - GitHub Actions context object - * @returns {Promise} */ -module.exports = async ({ github, context }) => { +async function addReviewersAsAssignees({ github, context }) { try { const prNumber = resolvePrNumber(context); if (!prNumber) return; @@ -118,10 +96,9 @@ module.exports = async ({ github, context }) => { if (requestedTeams.length > 0) { logger.info(`${requestedTeams.length} team reviewer(s) detected but ignored (only individual users are assigned)`); } + const usersToAssign = getUsersToAssign(requestedReviewers, currentAssignees); - const currentCount = currentAssignees.size; - const maxNewAssignees = Math.max(0, MAX_ASSIGNEES - currentCount); - const assigneesList = Array.from(usersToAssign).slice(0, maxNewAssignees); + const assigneesList = Array.from(usersToAssign); if (assigneesList.length === 0) { logger.log('No new users to assign. Done.'); @@ -129,7 +106,6 @@ module.exports = async ({ github, context }) => { } logger.log(`Will assign: ${assigneesList.join(', ')}`); - logAssigneeCapWarning(usersToAssign, maxNewAssignees); await github.rest.issues.addAssignees({ owner, @@ -148,4 +124,85 @@ module.exports = async ({ github, context }) => { } throw error; } +} + +/** + * 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, reviewer: explicitReviewer, prNumber: explicitPrNumber }) { + try { + 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.'); + return; + } + + const owner = context.repo.owner; + const repo = context.repo.repo; + + // 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); + + if (!currentAssignees.includes(reviewer)) { + logger.log(`${reviewer} is not an assignee on PR #${prNumber}. Nothing to remove.`); + return; + } + + logger.log(`Removing ${reviewer} from assignees on PR #${prNumber}`); + + await github.rest.issues.removeAssignees({ + owner, + repo, + issue_number: prNumber, + assignees: [reviewer] + }); + + logger.log(`✅ Successfully removed ${reviewer} from assignee(s)`); + + } catch (error) { + logger.error('Failed to remove assignee:', error.message); + if (error.status === 403) { + logger.warn(`403 returned: ${error.message}`); + return; + } + throw error; + } +} + +/** + * 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 + * @param {Object} params.context - GitHub Actions context object + * @returns {Promise} + */ +module.exports = async ({ github, context }) => { + const { eventName } = context; + const action = context.payload.action; + + if ( + (eventName === 'pull_request_target' && action === 'review_requested') || + eventName === 'workflow_dispatch' + ) { + await addReviewersAsAssignees({ github, context }); + } else { + logger.warn(`Unhandled event: ${eventName} / ${action}. Skipping.`); + } }; + +module.exports.removeReviewerFromAssignees = removeReviewerFromAssignees; diff --git a/.github/scripts/shared/helpers/constants.js b/.github/scripts/shared/helpers/constants.js index fc3d3cbc4..0703ec9c0 100644 --- a/.github/scripts/shared/helpers/constants.js +++ b/.github/scripts/shared/helpers/constants.js @@ -7,6 +7,5 @@ */ module.exports = { - MAX_ASSIGNEES: 2, BOT_NAME_ASSIGNEES: 'reviewers-assignee', }; diff --git a/.github/scripts/shared/helpers/reviewers-assignee-index.js b/.github/scripts/shared/helpers/reviewers-assignee-index.js index b7ab7273b..96586e2ee 100644 --- a/.github/scripts/shared/helpers/reviewers-assignee-index.js +++ b/.github/scripts/shared/helpers/reviewers-assignee-index.js @@ -10,6 +10,5 @@ const constants = require('./constants.js'); module.exports = { createLogger, - MAX_ASSIGNEES: constants.MAX_ASSIGNEES, BOT_NAME_ASSIGNEES: constants.BOT_NAME_ASSIGNEES, }; diff --git a/.github/workflows/capture-pr-review.yml b/.github/workflows/capture-pr-review.yml new file mode 100644 index 000000000..4b068ecea --- /dev/null +++ b/.github/workflows/capture-pr-review.yml @@ -0,0 +1,45 @@ +name: Bot - Capture PR Review + +on: + pull_request_review: + types: + - submitted + +permissions: {} + +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'); + const reviewer = context.payload.review?.user?.login; + const prNumber = context.payload.pull_request?.number; + if ( + typeof reviewer !== 'string' || + reviewer.length === 0 || + !Number.isInteger(prNumber) || + prNumber <= 0 + ) { + throw new Error(`Invalid pull_request_review payload: reviewer=${reviewer}, pr_number=${prNumber}`); + } + fs.mkdirSync('review-data', { recursive: true }); + fs.writeFileSync('review-data/review.json', JSON.stringify({ reviewer, pr_number: prNumber })); + + - 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 diff --git a/.github/workflows/on-review.yml b/.github/workflows/on-review.yml index 7743b7d0c..414ecec22 100644 --- a/.github/workflows/on-review.yml +++ b/.github/workflows/on-review.yml @@ -4,6 +4,11 @@ on: pull_request_target: types: - review_requested + workflow_run: + workflows: + - "Bot - Capture PR Review" + types: + - completed workflow_dispatch: inputs: pr_number: @@ -15,11 +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]') && + ( + (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 }} @@ -34,6 +46,7 @@ jobs: - name: Checkout repository uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: + ref: ${{ github.event.pull_request.base.sha || github.sha }} persist-credentials: false - name: Run Add Reviewers as Assignees @@ -42,3 +55,64 @@ 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' && + !contains(github.event.workflow_run.triggering_actor.login, '[bot]') + + steps: + - name: Harden Runner + uses: step-security/harden-runner@bf7454d06d71f1098171f2acdf0cd4708d7b5920 # v2.20.0 + with: + egress-policy: audit + + - name: Checkout repository + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + + - name: Check for review artifact + id: check + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + const name = `review-data-${context.payload.workflow_run.id}`; + const { data } = await github.rest.actions.listWorkflowRunArtifacts({ + owner: context.repo.owner, + repo: context.repo.repo, + run_id: context.payload.workflow_run.id + }); + const found = data.artifacts.some(a => a.name === name && !a.expired); + if (!found) core.notice('Review artifact not found; capture job was likely skipped. Nothing to do.'); + core.setOutput('found', String(found)); + + - name: Download review metadata + if: steps.check.outputs.found == 'true' + 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 }} + path: artifact + + - name: Run Remove Reviewer from Assignees + if: steps.check.outputs.found == 'true' + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + const fs = require('fs'); + const raw = JSON.parse(fs.readFileSync('artifact/review.json', 'utf8')); + const reviewer = typeof raw.reviewer === 'string' && /^[a-zA-Z0-9][a-zA-Z0-9-]*$/.test(raw.reviewer) + ? raw.reviewer : null; + const prNumber = Number.isInteger(raw.pr_number) && raw.pr_number > 0 + ? raw.pr_number : null; + if (!reviewer || !prNumber) { + core.setFailed(`Invalid artifact contents: reviewer=${raw.reviewer}, pr_number=${raw.pr_number}`); + return; + } + const { removeReviewerFromAssignees } = require('./.github/scripts/bot-pr-add-reviewers-as-assignees.js'); + await removeReviewerFromAssignees({ github, context, reviewer, prNumber });