Skip to content

feat: make reviewer-to-assignee bot dynamic with removal support #2391

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Mounil2005 wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

feat: make reviewer-to-assignee bot dynamic with removal support #2391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3a07014
feat: make reviewer-to-assignee bot dynamic with removal support
Mounil2005 Jun 27, 2026
86df148
fix: fetch live assignees before removal; isolate concurrency per rev…
Mounil2005 Jun 29, 2026
07e1e86
fix: use reviewer-scoped concurrency key for review-submission runs
Mounil2005 Jun 29, 2026
d3da720
fix: address reviewer feedback on concurrency, dead code, and bot skip
Mounil2005 Jul 10, 2026
d35a632
fix: pin checkout ref to base SHA to prevent fork code execution
Mounil2005 Jul 10, 2026
987990f
fix: use workflow_run to remove assignees with write token on fork PRs
Mounil2005 Jul 12, 2026
5d72e1d
fix: validate artifact contents before use to prevent artifact poisoning
Mounil2005 Jul 12, 2026
fb162b4
fix: validate payload before writing artifact; replace continue-on-er…
Mounil2005 Jul 12, 2026
bf256cc
fix: drop GITHUB_TOKEN from capture workflow (no repo access needed)
Mounil2005 Jul 12, 2026
0b0d4af
fix: checkout first, scope artifact download to subdirectory, skip bo…
Mounil2005 Jul 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
193 changes: 170 additions & 23 deletions .github/scripts/__tests__/jest/bot-pr-add-reviewers-as-assignees.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,12 @@

describe('Bot: Add Reviewers as Assignees', () => {

@exploreriii exploreriii Jul 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that you'd have a regex parse for the reviewer, you can test it will skip an invalid github name for the reviewer

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(() => {
Expand All @@ -16,6 +19,7 @@ describe('Bot: Add Reviewers as Assignees', () => {

const createTestState = () => ({
addAssigneesCalls: [],
removeAssigneesCalls: [],
pullsGetCalls: 0,
currentPrData: null,
});
Expand All @@ -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: [],
Expand All @@ -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: {
Expand All @@ -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 = {
Expand All @@ -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 = {
Expand Down Expand Up @@ -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: [] });
Expand All @@ -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: [] };

Expand All @@ -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 () => {
Expand All @@ -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 = {
Expand All @@ -194,15 +209,16 @@ describe('Bot: Add Reviewers as Assignees', () => {
const err = new Error('Forbidden');
err.status = 403;
throw err;
}
},
removeAssignees: async () => {}
}
}
};

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 = {
Expand All @@ -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);
});
});
Loading
Loading