Skip to content

Commit c1e28a3

Browse files
iron-progiron-prog
committed
fix: refetch issue state before final assignment decision (#2230)
The GFI and beginner /assign bots decided whether to assign based on context.payload.issue, a snapshot from the original webhook payload. Under concurrency, a queued run could still see a stale assignees/ labels/state and call addAssignees on an issue that was already assigned, closed, or relabeled. Both bots now refetch the issue via github.rest.issues.get() right before the final assignment decision and recheck assignees, state, and label against that fresh data. If the refetch fails, the bot aborts rather than decide on stale data. Regression tests added under __tests__/jest/ (repo's existing Jest harness, wired into test-scripts.yml) covering: stale-but-now-assigned, refetch failure, label removed, and issue closed — for both bots. Closes #2230
1 parent 92e7bcd commit c1e28a3

3 files changed

Lines changed: 288 additions & 8 deletions

File tree

‎.github/scripts/__tests__/jest/bot-assignment-fresh-issue.test.js‎

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
describe('assignment bots use fresh issue state before assigning', () => {
2+
let runGfiAssignBot;
3+
let runBeginnerAssignBot;
4+
5+
beforeAll(() => {
6+
runGfiAssignBot = require('../../bot-gfi-assign-on-comment.js');
7+
runBeginnerAssignBot = require('../../bot-beginner-assign-on-comment.js');
8+
});
9+
10+
beforeEach(() => {
11+
jest.clearAllMocks();
12+
});
13+
14+
function createContext({ labelName, payloadAssignees = [], freshLabels, freshAssignees = [], freshState = 'open' }) {
15+
const labels = [{ name: labelName }];
16+
17+
return {
18+
repo: {
19+
owner: 'hiero-ledger',
20+
repo: 'hiero-sdk-python',
21+
},
22+
payload: {
23+
repository: {
24+
owner: { login: 'hiero-ledger' },
25+
name: 'hiero-sdk-python',
26+
},
27+
issue: {
28+
number: 123,
29+
labels,
30+
assignees: payloadAssignees,
31+
},
32+
comment: {
33+
body: '/assign',
34+
user: {
35+
login: 'new-contributor',
36+
type: 'User',
37+
},
38+
},
39+
},
40+
freshIssue: {
41+
number: 123,
42+
title: 'Example issue',
43+
state: freshState,
44+
labels: freshLabels ?? labels,
45+
assignees: freshAssignees,
46+
},
47+
};
48+
}
49+
50+
function createGithubMock(context, { freshIssueError } = {}) {
51+
const calls = {
52+
comments: [],
53+
assignees: [],
54+
};
55+
56+
const github = {
57+
rest: {
58+
issues: {
59+
get: async () => {
60+
if (freshIssueError) {
61+
throw freshIssueError;
62+
}
63+
return { data: context.freshIssue };
64+
},
65+
createComment: async (params) => {
66+
calls.comments.push(params);
67+
return { data: {} };
68+
},
69+
addAssignees: async (params) => {
70+
calls.assignees.push(params);
71+
return { data: {} };
72+
},
73+
},
74+
},
75+
paginate: async () => [],
76+
graphql: async () => ({ search: { issueCount: 1 } }),
77+
};
78+
79+
return { github, calls };
80+
}
81+
82+
it('GFI bot does not assign when webhook payload is stale but issue is now assigned', async () => {
83+
const context = createContext({
84+
labelName: 'Good First Issue',
85+
payloadAssignees: [],
86+
freshAssignees: [{ login: 'current-assignee' }],
87+
});
88+
const { github, calls } = createGithubMock(context);
89+
90+
await runGfiAssignBot({ github, context });
91+
92+
expect(calls.assignees.length).toBe(0);
93+
expect(calls.comments.length).toBe(1);
94+
expect(calls.comments[0].body).toMatch(/already assigned/i);
95+
expect(calls.comments[0].body).toMatch(/@current-assignee/);
96+
});
97+
98+
it('GFI bot exits safely when fresh issue fetch fails before assignment', async () => {
99+
const context = createContext({
100+
labelName: 'Good First Issue',
101+
payloadAssignees: [],
102+
});
103+
const { github, calls } = createGithubMock(context, {
104+
freshIssueError: Object.assign(new Error('GitHub API unavailable'), { status: 503 }),
105+
});
106+
107+
await runGfiAssignBot({ github, context });
108+
109+
expect(calls.assignees.length).toBe(0);
110+
expect(calls.comments.length).toBe(0);
111+
});
112+
113+
it('GFI bot exits when the fresh issue is no longer a Good First Issue', async () => {
114+
const context = createContext({
115+
labelName: 'Good First Issue',
116+
payloadAssignees: [],
117+
freshLabels: [{ name: 'help wanted' }],
118+
});
119+
const { github, calls } = createGithubMock(context);
120+
121+
await runGfiAssignBot({ github, context });
122+
123+
expect(calls.assignees.length).toBe(0);
124+
expect(calls.comments.length).toBe(0);
125+
});
126+
127+
it('GFI bot exits when the fresh issue is closed before assignment', async () => {
128+
const context = createContext({
129+
labelName: 'Good First Issue',
130+
payloadAssignees: [],
131+
freshState: 'closed',
132+
});
133+
const { github, calls } = createGithubMock(context);
134+
135+
await runGfiAssignBot({ github, context });
136+
137+
expect(calls.assignees.length).toBe(0);
138+
expect(calls.comments.length).toBe(0);
139+
});
140+
141+
it('beginner bot does not assign when webhook payload is stale but issue is now assigned', async () => {
142+
const context = createContext({
143+
labelName: 'skill: beginner',
144+
payloadAssignees: [],
145+
freshAssignees: [{ login: 'current-assignee' }],
146+
});
147+
const { github, calls } = createGithubMock(context);
148+
149+
await runBeginnerAssignBot({ github, context });
150+
151+
expect(calls.assignees.length).toBe(0);
152+
expect(calls.comments.length).toBe(1);
153+
expect(calls.comments[0].body).toMatch(/already assigned/i);
154+
expect(calls.comments[0].body).toMatch(/@current-assignee/);
155+
});
156+
157+
it('beginner bot exits safely when fresh issue fetch fails before assignment', async () => {
158+
const context = createContext({
159+
labelName: 'skill: beginner',
160+
payloadAssignees: [],
161+
});
162+
const { github, calls } = createGithubMock(context, {
163+
freshIssueError: Object.assign(new Error('GitHub API unavailable'), { status: 503 }),
164+
});
165+
166+
await runBeginnerAssignBot({ github, context });
167+
168+
expect(calls.assignees.length).toBe(0);
169+
expect(calls.comments.length).toBe(0);
170+
});
171+
172+
it('beginner bot exits when the fresh issue no longer has the beginner label', async () => {
173+
const context = createContext({
174+
labelName: 'skill: beginner',
175+
payloadAssignees: [],
176+
freshLabels: [{ name: 'help wanted' }],
177+
});
178+
const { github, calls } = createGithubMock(context);
179+
180+
await runBeginnerAssignBot({ github, context });
181+
182+
expect(calls.assignees.length).toBe(0);
183+
expect(calls.comments.length).toBe(0);
184+
});
185+
186+
it('beginner bot exits when the fresh issue is closed before assignment', async () => {
187+
const context = createContext({
188+
labelName: 'skill: beginner',
189+
payloadAssignees: [],
190+
freshState: 'closed',
191+
});
192+
const { github, calls } = createGithubMock(context);
193+
194+
await runBeginnerAssignBot({ github, context });
195+
196+
expect(calls.assignees.length).toBe(0);
197+
expect(calls.comments.length).toBe(0);
198+
});
199+
});

‎.github/scripts/bot-beginner-assign-on-comment.js‎

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -256,9 +256,48 @@ Please try a GFI first, then come back — we’ll be happy to assign this! 😊
256256
}
257257

258258
// --- ASSIGNMENT LOGIC ---
259-
if (issue.assignees && issue.assignees.length > 0) {
259+
260+
// Refetch current issue state to avoid race conditions from a stale
261+
// webhook payload: an earlier queued run may have already assigned
262+
// this issue by the time this run executes.
263+
let freshIssue;
264+
try {
265+
const { data } = await github.rest.issues.get({
266+
owner: repo.owner.login,
267+
repo: repo.name,
268+
issue_number: issue.number,
269+
});
270+
freshIssue = data;
271+
} catch (error) {
272+
console.error("[Beginner Bot] Failed to refetch issue state, aborting to avoid a stale assignment decision:", {
273+
issue: issue.number,
274+
message: error.message,
275+
});
276+
return;
277+
}
278+
279+
console.log("[Beginner Bot] Fresh issue snapshot:", {
280+
state: freshIssue.state,
281+
assignees: freshIssue.assignees?.map((a) => a.login),
282+
});
283+
284+
if (freshIssue.state === "closed") {
285+
console.log(`[Beginner Bot] Issue #${issue.number} is closed. Skipping assignment.`);
286+
return;
287+
}
288+
289+
const freshHasBeginnerLabel =
290+
Array.isArray(freshIssue.labels) &&
291+
freshIssue.labels.some((label) => label.name?.toLowerCase() === beginnerLabel);
292+
293+
if (!freshHasBeginnerLabel) {
294+
console.log(`[Beginner Bot] Issue #${issue.number} no longer has '${BEGINNER_LABEL}' label. Exiting.`);
295+
return;
296+
}
297+
298+
if (freshIssue.assignees && freshIssue.assignees.length > 0) {
260299
try{
261-
const currentAssignee = issue.assignees[0]?.login ?? "another contributor";
300+
const currentAssignee = freshIssue.assignees[0]?.login ?? "another contributor";
262301
await github.rest.issues.createComment({
263302
owner: repo.owner.login,
264303
repo: repo.name,
@@ -401,4 +440,4 @@ Please try a GFI first, then come back — we’ll be happy to assign this! 😊
401440
comment: context.payload?.comment?.id
402441
});
403442
}
404-
};
443+
};

‎.github/scripts/bot-gfi-assign-on-comment.js‎

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -305,19 +305,61 @@ module.exports = async ({ github, context }) => {
305305
const requesterUsername = comment.user.login;
306306
const issueNumber = issue.number;
307307

308+
// -------------------------------
309+
// REFETCH ISSUE STATE
310+
// -------------------------------
311+
// context.payload.issue is a snapshot from the original issue_comment
312+
// webhook. In a queued/concurrent run, an earlier run may have already
313+
// assigned the issue by the time this run executes, so we refetch the
314+
// current issue state from the API instead of trusting the stale payload.
315+
let freshIssue;
316+
try {
317+
const { data } = await github.rest.issues.get({
318+
owner,
319+
repo,
320+
issue_number: issueNumber,
321+
});
322+
freshIssue = data;
323+
} catch (error) {
324+
console.error('[gfi-assign] Failed to refetch issue state, aborting to avoid a stale assignment decision:', {
325+
message: error.message,
326+
status: error.status,
327+
issueNumber,
328+
});
329+
return;
330+
}
331+
332+
console.log('[gfi-assign] Fresh issue snapshot:', {
333+
state: freshIssue.state,
334+
assignees: freshIssue.assignees?.map(a => a.login),
335+
labels: freshIssue.labels?.map(l => l.name ?? l),
336+
});
337+
338+
// Reject if the issue was closed between the comment and now
339+
if (freshIssue.state === 'closed') {
340+
console.log('[gfi-assign] Exit: issue is closed');
341+
return;
342+
}
343+
344+
// Reject if the Good First Issue label was removed between the comment and now
345+
if (!issueIsGoodFirstIssue(freshIssue)) {
346+
console.log('[gfi-assign] Exit: issue no longer labeled Good First Issue');
347+
return;
348+
}
349+
308350
console.log('[gfi-assign] Requester:', requesterUsername);
309-
console.log('[gfi-assign] Current assignees:', issue.assignees?.map(a => a.login));
351+
console.log('[gfi-assign] Current assignees (fresh):', freshIssue.assignees?.map(a => a.login));
310352

311-
// Reject if issue is already assigned
353+
// Reject if issue is already assigned (checked against fresh state)
312354
// Comment failure to the requester
313-
if (issue.assignees?.length > 0) {
355+
if (freshIssue.assignees?.length > 0) {
314356
console.log('[gfi-assign] Exit: issue already assigned');
315357

316358
await github.rest.issues.createComment({
317359
owner,
318360
repo,
319361
issue_number: issueNumber,
320-
body: commentAlreadyAssigned(requesterUsername, issue),
362+
body: commentAlreadyAssigned(requesterUsername, freshIssue),
321363
});
322364

323365
console.log('[gfi-assign] Posted already-assigned comment');
@@ -433,4 +475,4 @@ module.exports = async ({ github, context }) => {
433475
});
434476
throw error;
435477
}
436-
};
478+
};

0 commit comments

Comments
 (0)