fix: refetch issue state before final assignment decision #2431
fix: refetch issue state before final assignment decision #2431iron-prog wants to merge 1 commit into
Conversation
iron-prog
changed the title
…er#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 hiero-ledger#2230 Signed-off-by: iron-prog <dt915725@gmail.com>
c1e28a3 to
3eb0bc9
Compare
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughBoth assignment bots now refetch issue data before assigning, validate the fresh issue state and labels, and use current assignees for duplicate-assignment handling. A Jest suite covers stale payloads, fetch failures, removed labels, and closed issues. ChangesFresh Issue Assignment
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Requester
participant AssignmentBot
participant GitHubREST
Requester->>AssignmentBot: Send /assign comment
AssignmentBot->>GitHubREST: Fetch current issue
GitHubREST-->>AssignmentBot: Return fresh labels, state, and assignees
AssignmentBot->>GitHubREST: Create comment or add assignee
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/scripts/bot-gfi-assign-on-comment.js (2)
355-367: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWrap the "already assigned"
createCommentin a granular try/catch for resilience.The beginner bot wraps this same operation in a try/catch (lines 299–313), logging a contextual error and continuing. Here, a
createCommentfailure propagates to the outer catch which re-throws, failing the workflow over a non-critical notification. As per coding guidelines for.github/scripts/**/*.js, async operations should be wrapped in try/catch with contextual errors.🛡️ Proposed fix
if (freshIssue.assignees?.length > 0) { console.log('[gfi-assign] Exit: issue already assigned'); - await github.rest.issues.createComment({ - owner, - repo, - issue_number: issueNumber, - body: commentAlreadyAssigned(requesterUsername, freshIssue), - }); + try { + await github.rest.issues.createComment({ + owner, + repo, + issue_number: issueNumber, + body: commentAlreadyAssigned(requesterUsername, freshIssue), + }); + console.log('[gfi-assign] Posted already-assigned comment'); + } catch (error) { + console.error('[gfi-assign] Failed to post already-assigned comment:', { + message: error.message, + status: error.status, + issueNumber, + }); + } console.log('[gfi-assign] Posted already-assigned comment'); return; }Source: Path instructions
308-367: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRefetch logic is duplicated across both bots with divergent error handling. Both bots independently implement the same refetch-then-validate pattern (fetch fresh issue → abort on error → check closed → check label → check assignees), but with inconsistent error handling: the GFI bot's "already assigned"
createCommentis not wrapped in a granular try/catch (error re-throws, failing the workflow), while the beginner bot's equivalent is wrapped and logged gracefully. Extracting a shared helper would eliminate this duplication and enforce consistent error handling.
.github/scripts/bot-gfi-assign-on-comment.js#L308-L367: Extract the refetch + closed/label/assignee validation into a shared helper (e.g.,fetchFreshIssueState), parameterizing the label-check function and "already assigned" comment body. Wrap thecreateCommentat line 358 in a granular try/catch within the helper or at the call site..github/scripts/bot-beginner-assign-on-comment.js#L259-L315: Replace the inline refetch logic with a call to the shared helper, passing the beginner label check and already-assigned comment builder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d8d765b-efe8-41e1-99ac-947429732cbf
📒 Files selected for processing (3)
.github/scripts/__tests__/jest/bot-assignment-fresh-issue.test.js.github/scripts/bot-beginner-assign-on-comment.js.github/scripts/bot-gfi-assign-on-comment.js
| 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 }); | ||
|
|
||
| expect(calls.assignees.length).toBe(0); | ||
| expect(calls.comments.length).toBe(1); | ||
| expect(calls.comments[0].body).toMatch(/already assigned/i); | ||
| expect(calls.comments[0].body).toMatch(/@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 }); | ||
|
|
||
| expect(calls.assignees.length).toBe(0); | ||
| expect(calls.comments.length).toBe(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 }); | ||
|
|
||
| expect(calls.assignees.length).toBe(0); | ||
| expect(calls.comments.length).toBe(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 }); | ||
|
|
||
| expect(calls.assignees.length).toBe(0); | ||
| expect(calls.comments.length).toBe(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 }); | ||
|
|
||
| expect(calls.assignees.length).toBe(0); | ||
| expect(calls.comments.length).toBe(1); | ||
| expect(calls.comments[0].body).toMatch(/already assigned/i); | ||
| expect(calls.comments[0].body).toMatch(/@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 }); | ||
|
|
||
| expect(calls.assignees.length).toBe(0); | ||
| expect(calls.comments.length).toBe(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 }); | ||
|
|
||
| expect(calls.assignees.length).toBe(0); | ||
| expect(calls.comments.length).toBe(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 }); | ||
|
|
||
| expect(calls.assignees.length).toBe(0); | ||
| expect(calls.comments.length).toBe(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a positive test case verifying the bot assigns when all conditions are valid.
All 8 test cases cover negative scenarios (bot should NOT assign). A positive case — fresh issue is open, labeled, unassigned, and under the assignment limit — would verify the refetch logic doesn't break the happy path and that addAssignees is called with the correct parameters.
♻️ Suggested positive test cases
+ it('GFI bot assigns when fresh issue is open, labeled, and unassigned', async () => {
+ const context = createContext({
+ labelName: 'Good First Issue',
+ payloadAssignees: [],
+ freshAssignees: [],
+ });
+ const { github, calls } = createGithubMock(context);
+
+ await runGfiAssignBot({ github, context });
+
+ expect(calls.assignees.length).toBe(1);
+ expect(calls.assignees[0].assignees).toEqual(['new-contributor']);
+ });
+
+ it('beginner bot assigns when fresh issue is open, labeled, and unassigned', async () => {
+ const context = createContext({
+ labelName: 'skill: beginner',
+ payloadAssignees: [],
+ freshAssignees: [],
+ });
+ const { github, calls } = createGithubMock(context);
+
+ await runBeginnerAssignBot({ github, context });
+
+ expect(calls.assignees.length).toBe(1);
+ expect(calls.assignees[0].assignees).toEqual(['new-contributor']);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 }); | |
| expect(calls.assignees.length).toBe(0); | |
| expect(calls.comments.length).toBe(1); | |
| expect(calls.comments[0].body).toMatch(/already assigned/i); | |
| expect(calls.comments[0].body).toMatch(/@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 }); | |
| expect(calls.assignees.length).toBe(0); | |
| expect(calls.comments.length).toBe(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 }); | |
| expect(calls.assignees.length).toBe(0); | |
| expect(calls.comments.length).toBe(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 }); | |
| expect(calls.assignees.length).toBe(0); | |
| expect(calls.comments.length).toBe(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 }); | |
| expect(calls.assignees.length).toBe(0); | |
| expect(calls.comments.length).toBe(1); | |
| expect(calls.comments[0].body).toMatch(/already assigned/i); | |
| expect(calls.comments[0].body).toMatch(/@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 }); | |
| expect(calls.assignees.length).toBe(0); | |
| expect(calls.comments.length).toBe(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 }); | |
| expect(calls.assignees.length).toBe(0); | |
| expect(calls.comments.length).toBe(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 }); | |
| expect(calls.assignees.length).toBe(0); | |
| expect(calls.comments.length).toBe(0); | |
| }); | |
| }); | |
| 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 }); | |
| expect(calls.assignees.length).toBe(0); | |
| expect(calls.comments.length).toBe(1); | |
| expect(calls.comments[0].body).toMatch(/already assigned/i); | |
| expect(calls.comments[0].body).toMatch(/@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 }); | |
| expect(calls.assignees.length).toBe(0); | |
| expect(calls.comments.length).toBe(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 }); | |
| expect(calls.assignees.length).toBe(0); | |
| expect(calls.comments.length).toBe(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 }); | |
| expect(calls.assignees.length).toBe(0); | |
| expect(calls.comments.length).toBe(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 }); | |
| expect(calls.assignees.length).toBe(0); | |
| expect(calls.comments.length).toBe(1); | |
| expect(calls.comments[0].body).toMatch(/already assigned/i); | |
| expect(calls.comments[0].body).toMatch(/@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 }); | |
| expect(calls.assignees.length).toBe(0); | |
| expect(calls.comments.length).toBe(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 }); | |
| expect(calls.assignees.length).toBe(0); | |
| expect(calls.comments.length).toBe(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 }); | |
| expect(calls.assignees.length).toBe(0); | |
| expect(calls.comments.length).toBe(0); | |
| }); | |
| }); |
github-actions
Bot
added
open to community review
|
|
||
| // --- ASSIGNMENT LOGIC --- | ||
| if (issue.assignees && issue.assignees.length > 0) { | ||
|
|
There was a problem hiding this comment.
The new "refetch → check closed → check label → check assignees" block (~25-30 lines) is added twice....once here, once in bot-gfi-assign-on-comment.js (lines 309-355)...nearly identical in structure. A shared helper (e.g. refetchIssueOrAbort(github, owner, repo, issueNumber, logPrefix), following the existing shared/labels.js pattern already used by both files) would collapse this into one implementation instead of two.
The two copies have already diverged slightly: the GFI bot's catch-block logs {message, status, issueNumber} (line 324), this one only logs {issue, message}, no status (line 272). Small, but it's the kind of drift that happens specifically because the logic is duplicated rather than shared.
|
The jest suite mocks github.rest.issues.get(), so it verifies the logic branches correctly but doesn't exercise the real GitHub Actions run end-to-end. Did you test this on a fork? Labeling a test issue, commenting /assign, and confirming the flow works against the real API? If you have a fork where you already ran through it, could you share the link? (Side note: both workflows already have a concurrency: group that serializes runs per repo/issue, so this refetch is the complete fix for the race in #2230, not just a mitigation, permissions are fine too, issues: write already covers the new get() call. Just want to see the actual end-to-end flow run once on a live fork before merge.) |
|
HI @iron-prog Thank you very much for submitting this work. I would like to apologize for the confusion. The issue is currently assigned to someone and should not have been assigned to more then one person without having an initial conversation with both parties. Therefore I will need to close this PR for now. If the issue is deemed free then this PR can be reopened. :) Thank you! |
5 participants
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
Description:
Refetch current issue state from the GitHub API before the GFI and beginner assignment bots make their final
addAssigneesdecision, instead of trusting the original webhook payload.github.rest.issues.get()immediately before assignment in the GFI botgithub.rest.issues.get()immediately before assignment in the beginner bot.github/scripts/__tests__/jest/(the repo's existing Jest harness, wired intotest-scripts.yml) covering: stale-but-now-assigned, refetch failure, label removed, and issue closed — for both botsNotes for reviewer:
The GFI and beginner
/assignbots previously decided whether to assign based oncontext.payload.issue, a snapshot from the originalissue_commentwebhook payload. Under concurrency, a queued run could still see stale assignees/labels/state and calladdAssigneeson an issue that had already been assigned, closed, or relabeled by an earlier run.Ran the full existing test suite locally after these changes — all passing, no regressions:
Related issue(s):
Fixes #2230
Notes for reviewer:
Checklist