-
Notifications
You must be signed in to change notification settings - Fork 283
fix: refetch current issue state before assignment bots assign users #2237
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
c71e516
b7e4bef
ae81c81
2ee160b
bbc3e40
4522767
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,195 @@ | ||
| // Regression coverage for stale issue_comment payloads: the bots must refetch issue state before assigning. | ||
|
|
||
| const { describe, it } = require('node:test'); | ||
| const assert = require('node:assert/strict'); | ||
|
|
||
| const runGfiAssignBot = require('./bot-gfi-assign-on-comment.js'); | ||
| const runBeginnerAssignBot = require('./bot-beginner-assign-on-comment.js'); | ||
|
|
||
| function createContext({ labelName, payloadAssignees = [], freshLabels, freshAssignees = [], freshState = 'open' }) { | ||
| const labels = [{ name: labelName }]; | ||
|
|
||
| return { | ||
| repo: { | ||
| owner: 'hiero-ledger', | ||
| repo: 'hiero-sdk-python', | ||
| }, | ||
| payload: { | ||
| repository: { | ||
| owner: { login: 'hiero-ledger' }, | ||
| name: 'hiero-sdk-python', | ||
| }, | ||
| issue: { | ||
| number: 123, | ||
| labels, | ||
| assignees: payloadAssignees, | ||
| }, | ||
| comment: { | ||
| body: '/assign', | ||
| user: { | ||
| login: 'new-contributor', | ||
| type: 'User', | ||
| }, | ||
| }, | ||
| }, | ||
| freshIssue: { | ||
| number: 123, | ||
| title: 'Example issue', | ||
| state: freshState, | ||
| labels: freshLabels ?? labels, | ||
| assignees: freshAssignees, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| function createGithubMock(context, { freshIssueError } = {}) { | ||
| const calls = { | ||
| comments: [], | ||
| assignees: [], | ||
| }; | ||
|
|
||
| const github = { | ||
| rest: { | ||
| issues: { | ||
| get: async () => { | ||
| if (freshIssueError) { | ||
| throw freshIssueError; | ||
| } | ||
| return { data: context.freshIssue }; | ||
| }, | ||
| createComment: async (params) => { | ||
| calls.comments.push(params); | ||
| return { data: {} }; | ||
| }, | ||
| addAssignees: async (params) => { | ||
| calls.assignees.push(params); | ||
| return { data: {} }; | ||
| }, | ||
| }, | ||
| }, | ||
| paginate: async () => [], | ||
| graphql: async () => ({ search: { issueCount: 1 } }), | ||
| }; | ||
|
|
||
| return { github, calls }; | ||
| } | ||
|
|
||
| describe('assignment bots use fresh issue state before assigning', () => { | ||
| it('GFI bot does not assign when webhook payload is stale but issue is now assigned', async () => { | ||
| const context = createContext({ | ||
| labelName: 'Good First Issue', | ||
| payloadAssignees: [], | ||
| freshAssignees: [{ login: 'current-assignee' }], | ||
| }); | ||
| const { github, calls } = createGithubMock(context); | ||
|
|
||
| await runGfiAssignBot({ github, context }); | ||
|
|
||
| assert.equal(calls.assignees.length, 0); | ||
| assert.equal(calls.comments.length, 1); | ||
| assert.match(calls.comments[0].body, /already assigned/i); | ||
| assert.match(calls.comments[0].body, /@current-assignee/); | ||
| }); | ||
|
|
||
| it('GFI bot exits safely when fresh issue fetch fails before assignment', async () => { | ||
| const context = createContext({ | ||
| labelName: 'Good First Issue', | ||
| payloadAssignees: [], | ||
| }); | ||
| const { github, calls } = createGithubMock(context, { | ||
| freshIssueError: Object.assign(new Error('GitHub API unavailable'), { status: 503 }), | ||
| }); | ||
|
|
||
| await runGfiAssignBot({ github, context }); | ||
|
|
||
| assert.equal(calls.assignees.length, 0); | ||
| assert.equal(calls.comments.length, 0); | ||
| }); | ||
|
|
||
| it('GFI bot exits when the fresh issue is no longer a Good First Issue', async () => { | ||
| const context = createContext({ | ||
| labelName: 'Good First Issue', | ||
| payloadAssignees: [], | ||
| freshLabels: [{ name: 'help wanted' }], | ||
| }); | ||
| const { github, calls } = createGithubMock(context); | ||
|
|
||
| await runGfiAssignBot({ github, context }); | ||
|
|
||
| assert.equal(calls.assignees.length, 0); | ||
| assert.equal(calls.comments.length, 0); | ||
| }); | ||
|
|
||
| it('GFI bot exits when the fresh issue is closed before assignment', async () => { | ||
| const context = createContext({ | ||
| labelName: 'Good First Issue', | ||
| payloadAssignees: [], | ||
| freshState: 'closed', | ||
| }); | ||
| const { github, calls } = createGithubMock(context); | ||
|
|
||
| await runGfiAssignBot({ github, context }); | ||
|
|
||
| assert.equal(calls.assignees.length, 0); | ||
| assert.equal(calls.comments.length, 0); | ||
| }); | ||
|
|
||
| it('beginner bot does not assign when webhook payload is stale but issue is now assigned', async () => { | ||
| const context = createContext({ | ||
| labelName: 'skill: beginner', | ||
| payloadAssignees: [], | ||
| freshAssignees: [{ login: 'current-assignee' }], | ||
| }); | ||
| const { github, calls } = createGithubMock(context); | ||
|
|
||
| await runBeginnerAssignBot({ github, context }); | ||
|
|
||
| assert.equal(calls.assignees.length, 0); | ||
| assert.equal(calls.comments.length, 1); | ||
| assert.match(calls.comments[0].body, /already assigned/i); | ||
| assert.match(calls.comments[0].body, /@current-assignee/); | ||
| }); | ||
|
|
||
| it('beginner bot exits safely when fresh issue fetch fails before assignment', async () => { | ||
| const context = createContext({ | ||
| labelName: 'skill: beginner', | ||
| payloadAssignees: [], | ||
| }); | ||
| const { github, calls } = createGithubMock(context, { | ||
| freshIssueError: Object.assign(new Error('GitHub API unavailable'), { status: 503 }), | ||
| }); | ||
|
|
||
| await runBeginnerAssignBot({ github, context }); | ||
|
|
||
| assert.equal(calls.assignees.length, 0); | ||
| assert.equal(calls.comments.length, 0); | ||
| }); | ||
|
|
||
| it('beginner bot exits when the fresh issue no longer has the beginner label', async () => { | ||
| const context = createContext({ | ||
| labelName: 'skill: beginner', | ||
| payloadAssignees: [], | ||
| freshLabels: [{ name: 'help wanted' }], | ||
| }); | ||
| const { github, calls } = createGithubMock(context); | ||
|
|
||
| await runBeginnerAssignBot({ github, context }); | ||
|
|
||
| assert.equal(calls.assignees.length, 0); | ||
| assert.equal(calls.comments.length, 0); | ||
| }); | ||
|
|
||
| it('beginner bot exits when the fresh issue is closed before assignment', async () => { | ||
| const context = createContext({ | ||
| labelName: 'skill: beginner', | ||
| payloadAssignees: [], | ||
| freshState: 'closed', | ||
| }); | ||
| const { github, calls } = createGithubMock(context); | ||
|
|
||
| await runBeginnerAssignBot({ github, context }); | ||
|
|
||
| assert.equal(calls.assignees.length, 0); | ||
| assert.equal(calls.comments.length, 0); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,6 +86,16 @@ function getCurrentAssigneeMention(issue) { | |
| return login ? `@${login}` : 'someone'; | ||
| } | ||
|
|
||
| async function getFreshIssue({ github, owner, repo, issueNumber }) { | ||
| const { data } = await github.rest.issues.get({ | ||
| owner, | ||
| repo, | ||
| issue_number: issueNumber, | ||
| }); | ||
|
|
||
| return data; | ||
| } | ||
|
|
||
| /** | ||
| * Builds a comment explaining that the issue is already assigned. | ||
| * Requester username is passed from main | ||
|
|
@@ -361,7 +371,54 @@ module.exports = async ({ github, context }) => { | |
| return; | ||
| } | ||
|
|
||
| console.log('[gfi-assign] Assigning issue to requester'); | ||
| console.log('[gfi-assign] Checking current issue state before assignment'); | ||
|
|
||
| let currentIssue; | ||
| try { | ||
| currentIssue = await getFreshIssue({ | ||
| github, | ||
| owner, | ||
| repo, | ||
| issueNumber, | ||
| }); | ||
|
chaitanyamedidar marked this conversation as resolved.
|
||
| } catch (error) { | ||
| console.error('[gfi-assign] Failed to fetch current issue state before assignment:', { | ||
| message: error.message, | ||
| status: error.status, | ||
| owner, | ||
| repo, | ||
| issueNumber, | ||
| requesterUsername, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (!issueIsGoodFirstIssue(currentIssue)) { | ||
| console.log('[gfi-assign] Exit: current issue state is no longer a Good First Issue'); | ||
| return; | ||
| } | ||
|
|
||
| if (currentIssue.state !== 'open') { | ||
| console.log('[gfi-assign] Exit: current issue state is not open', { | ||
| issueNumber, | ||
| state: currentIssue.state, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (currentIssue.assignees?.length > 0) { | ||
| console.log('[gfi-assign] Exit: current issue state is already assigned'); | ||
|
|
||
| await github.rest.issues.createComment({ | ||
| owner, | ||
| repo, | ||
| issue_number: issueNumber, | ||
| body: commentAlreadyAssigned(requesterUsername, currentIssue), | ||
| }); | ||
|
|
||
| console.log('[gfi-assign] Posted already-assigned comment from current issue state'); | ||
| return; | ||
| } | ||
|
Comment on lines
+409
to
+421
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win Wrap the createComment call in try/catch for consistency with the beginner bot. The beginner bot wraps its corresponding createComment call in try/catch (lines 378-391 in bot-beginner-assign-on-comment.js), gracefully handling comment failures without propagating the error. This call should follow the same pattern — if posting the "already assigned" comment fails, the bot should exit silently since the critical goal (avoiding incorrect assignment) has already been achieved. 🛡️ Proposed fix to add try/catch if (currentIssue.assignees?.length > 0) {
console.log('[gfi-assign] Exit: current issue state is already assigned');
+ try {
await github.rest.issues.createComment({
owner,
repo,
issue_number: issueNumber,
body: commentAlreadyAssigned(requesterUsername, currentIssue),
});
-
console.log('[gfi-assign] Posted already-assigned comment from current issue state');
+ } catch (error) {
+ console.error('[gfi-assign] Failed to post already-assigned message:', {
+ message: error.message,
+ issueNumber,
+ requesterUsername,
+ });
+ }
return;
} |
||
|
|
||
| // All validations passed and user has requested assignment on a GFI | ||
| // Assign the issue to the requester | ||
|
|
@@ -409,7 +466,7 @@ module.exports = async ({ github, context }) => { | |
| if (planExists) { | ||
| console.log('[gfi-assign] CodeRabbit plan already exists, skipping'); | ||
| } else { | ||
| await triggerCodeRabbitPlan(github, owner, repo, issue); | ||
| await triggerCodeRabbitPlan(github, owner, repo, currentIssue); | ||
| console.log('[gfi-assign] CodeRabbit plan chained successfully'); | ||
| } | ||
| } catch (error) { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.