Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
195 changes: 195 additions & 0 deletions .github/scripts/bot-assignment-fresh-issue.test.js
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);
});
});
Comment thread
chaitanyamedidar marked this conversation as resolved.
75 changes: 70 additions & 5 deletions .github/scripts/bot-beginner-assign-on-comment.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,27 @@ async function countCompletedGfiIssues(github, owner, repo, username) {
return result?.search?.issueCount ?? 0;
}

function issueHasBeginnerLabel(issue) {
const beginnerLabel = BEGINNER_LABEL.toLowerCase();
return Array.isArray(issue?.labels) && issue.labels.some((label) => label.name?.toLowerCase() === beginnerLabel);
}

async function getFreshIssue(github, owner, repo, issueNumber) {
const { data } = await github.rest.issues.get({
owner,
repo,
issue_number: issueNumber,
});

return data;
}

function buildAlreadyAssignedComment({ commenter, issue, repo }) {
const currentAssignee = issue.assignees?.[0]?.login ?? "another contributor";

return `👋 Hi @${commenter}, thanks for your interest! This issue is already assigned to @${currentAssignee}, but we'd love your help on another one. You can find more "${BEGINNER_LABEL}" issues [here](https://github.com/${repo.owner.login}/${repo.name}/issues?q=is%3Aissue+is%3Aopen+label%3A%22${encodeURIComponent(BEGINNER_LABEL)}%22+no%3Aassignee).`;
}

module.exports = async ({ github, context }) => {
try {
const { payload } = context;
Expand All @@ -116,8 +137,7 @@ module.exports = async ({ github, context }) => {
}

// 2. Label Check (Fix 2: Defensive Check)
const beginnerLabel = BEGINNER_LABEL.toLowerCase();
const hasBeginnerLabel = Array.isArray(issue.labels) && issue.labels.some((label) => label.name?.toLowerCase() === beginnerLabel);
const hasBeginnerLabel = issueHasBeginnerLabel(issue);
if (!hasBeginnerLabel) {
console.log(`[Beginner Bot] Issue #${issue.number} does not have '${BEGINNER_LABEL}' label. Exiting.`);
return;
Expand Down Expand Up @@ -258,12 +278,11 @@ Please try a GFI first, then come back — we’ll be happy to assign this! 😊
// --- ASSIGNMENT LOGIC ---
if (issue.assignees && issue.assignees.length > 0) {
try{
const currentAssignee = issue.assignees[0]?.login ?? "another contributor";
await github.rest.issues.createComment({
owner: repo.owner.login,
repo: repo.name,
issue_number: issue.number,
body: `👋 Hi @${commenter}, thanks for your interest! This issue is already assigned to @${currentAssignee}, but we'd love your help on another one. You can find more "${BEGINNER_LABEL}" issues [here](https://github.com/${repo.owner.login}/${repo.name}/issues?q=is%3Aissue+is%3Aopen+label%3A%22${encodeURIComponent(BEGINNER_LABEL)}%22+no%3Aassignee).`,
body: buildAlreadyAssignedComment({ commenter, issue, repo }),
});
} catch (error) {
console.error("[Beginner Bot] Failed to post already-assigned message:", {
Expand Down Expand Up @@ -325,7 +344,53 @@ Please try a GFI first, then come back — we’ll be happy to assign this! 😊
return;
}

console.log(`[Beginner Bot] Assigning issue #${issue.number} to @${commenter}...`);
console.log(`[Beginner Bot] Checking current issue state before assigning #${issue.number} to @${commenter}...`);

let currentIssue;
try {
currentIssue = await getFreshIssue(
github,
repo.owner.login,
repo.name,
issue.number
);
} catch (error) {
console.error("[Beginner Bot] Failed to fetch current issue state before assignment:", {
issue: issue.number,
commenter,
message: error.message,
status: error.status,
});
return;
}

if (!issueHasBeginnerLabel(currentIssue)) {
console.log(`[Beginner Bot] Issue #${issue.number} no longer has '${BEGINNER_LABEL}' label. Exiting.`);
return;
}

if (currentIssue.state !== "open") {
console.log(`[Beginner Bot] Issue #${issue.number} is '${currentIssue.state}'. Exiting.`);
return;
}

if (currentIssue.assignees && currentIssue.assignees.length > 0) {
try{
await github.rest.issues.createComment({
owner: repo.owner.login,
repo: repo.name,
issue_number: issue.number,
body: buildAlreadyAssignedComment({ commenter, issue: currentIssue, repo }),
});
} catch (error) {
console.error("[Beginner Bot] Failed to post already-assigned message:", {
issue: issue.number,
commenter,
message: error.message,
});
}
return;
}

// Fix 4: Granular Try/Catch for Assign API
try {
Expand Down
61 changes: 59 additions & 2 deletions .github/scripts/bot-gfi-assign-on-comment.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
});
Comment thread
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
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.

🧹 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
Expand Down Expand Up @@ -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) {
Expand Down