fix: refetch current issue state before assignment bots assign users#2237
fix: refetch current issue state before assignment bots assign users#2237chaitanyamedidar wants to merge 6 commits into
Conversation
Signed-off-by: chaitanyamedidar <175808937+chaitanyamedidar@users.noreply.github.com>
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 18 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
WalkthroughBoth assignment bot scripts refetch the current issue via the GitHub API before final assignment checks; they validate labels, assignees, and issue state from the fresh issue, post an "already assigned" comment and exit when appropriate, and the GFI flow forwards the refreshed issue to downstream triggering. ChangesAssignment Bot Fresh State Validation
Sequence Diagram(s)sequenceDiagram
participant User as Commenter
participant Bot as AssignmentBot
participant GH as GitHubAPI
participant Plan as CodeRabbitPlan
Note over User,Bot: Webhook payload (may be stale)
User->>Bot: POST /assign comment (webhook)
Bot->>GH: GET /issues/:number (getFreshIssue) rgba(100,150,240,0.5)
GH-->>Bot: issue (fresh state: labels, assignees, state)
alt label missing or not matching
Bot->>Bot: exit early (no assignment)
else already assigned
Bot->>GH: POST /issues/:number/comments ("already assigned" `@assignee`)
else unassigned & label present
Bot->>GH: POST /issues/:number/assignees (addAssignees)
Bot->>Plan: POST trigger with refreshed issue
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
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 (1)
396-423:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a refreshed open-state check before final assignment write.
Line 396 revalidates labels and Line 401 revalidates assignees, but
currentIssue.stateis not checked beforeaddAssignees. A delayed run can still assign a closed issue.Suggested patch
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');.github/scripts/bot-beginner-assign-on-comment.js (1)
362-393:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate on refreshed
issue.statebeforeaddAssignees.Line 362 checks refreshed label and Line 367 checks refreshed assignees, but there is no open-state check before assignment. In queued runs, this can still assign a closed issue.
Suggested patch
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) {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cc8e6c03-8c02-45b5-bad1-372dbc242d78
📒 Files selected for processing (2)
.github/scripts/bot-beginner-assign-on-comment.js.github/scripts/bot-gfi-assign-on-comment.js
There was a problem hiding this comment.
Pull request overview
This PR hardens the GitHub Actions assignment bots against stale issue_comment.created webhook payloads by refetching the current issue state immediately before performing the final addAssignees write.
Changes:
- Add a “fresh issue state” refetch in the GFI assignment bot before assigning, and re-check labels/assignees.
- Add a similar refetch + re-check flow in the beginner assignment bot (including a helper for beginner label detection).
- On refetch failure, exit safely without attempting assignment.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/scripts/bot-gfi-assign-on-comment.js | Refetches current issue state before assignment; revalidates GFI label + assignees; uses fresh issue object for follow-on CodeRabbit trigger. |
| .github/scripts/bot-beginner-assign-on-comment.js | Adds beginner-label helper and refetches current issue state before assignment to avoid decisions based on stale webhook data. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @explorer3, I checked the C++ assignment workflow and saw it already uses a similar safety pattern to what we discussed for Python: it refetches the current issue state before the final assignment write, so queued I’ve implemented the Python fix in the same small scope and added a focused regression test for the stale-payload case. Would you prefer that test file to be included in the PR as repo-side regression coverage, or should I keep the PR limited to the two bot script changes? |
|
Hello, this is the OfficeHourBot. This is a reminder that the Hiero Python SDK Office Hours are scheduled in approximately 4 hours (14:00 UTC). This session provides an opportunity to ask questions regarding this Pull Request. Details:
Disclaimer: This is an automated reminder. Please verify the schedule here for any changes. From, |
Signed-off-by: chaitanyamedidar <175808937+chaitanyamedidar@users.noreply.github.com>
exploreriii
added
step: 1st
|
LGTM ! Everything looks fine to me. Just few notes we should consider for future polish : Expanded Test Coverage
|
Signed-off-by: chaitanyamedidar <175808937+chaitanyamedidar@users.noreply.github.com>
|
Hey @darshit2308 , thanks for the suggestion. This will document the concurrency edge cases directly in tests, which is valuable because this PR is specifically about stale webhook state Added focused coverage for both defensive paths in |
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-beginner-assign-on-comment.js (2)
279-295:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid making the “already assigned” decision from stale webhook state.
At Line [279], the bot returns based on
context.payload.issue.assigneesbefore the fresh fetch at Line [351]. If the webhook snapshot is stale (assignee removed later),/assignis incorrectly blocked.Suggested fix
- // --- ASSIGNMENT LOGIC --- - if (issue.assignees && issue.assignees.length > 0) { - try{ - await github.rest.issues.createComment({ - owner: repo.owner.login, - repo: repo.name, - issue_number: issue.number, - body: buildAlreadyAssignedComment({ commenter, issue, repo }), - }); - } catch (error) { - console.error("[Beginner Bot] Failed to post already-assigned message:", { - issue: issue.number, - commenter, - message: error.message, - }); - } - return; - } + // --- ASSIGNMENT LOGIC --- + // Defer assignee decision until after fresh issue fetch.
367-397:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRevalidate fresh issue
statebefore callingaddAssignees.Lines [367]-[397] recheck label and assignees, but not
currentIssue.state. A closed issue can still pass this gate and be assigned.Suggested fix
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) {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5cef04dd-d5ea-4218-b763-b999603d8292
📒 Files selected for processing (2)
.github/scripts/bot-assignment-fresh-issue.test.js.github/scripts/bot-beginner-assign-on-comment.js
Signed-off-by: chaitanyamedidar <175808937+chaitanyamedidar@users.noreply.github.com>
github-actions
Bot
added
the
queue:junior-committer
exploreriii
removed
step: 1st
github-actions
Bot
added
the
open to community review
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
@chaitanyamedidar Do you have links to your testing?
|
@aceppaluni I tested it locally with: The focused regression test is here: This uses mocked Octokit/event replay, so the stale payload case is deterministic. I can also paste the local test command/output here if that would be more useful. |
|
@chaitanyamedidar Yes, if you could paste the output this would be appreciated! It helps to ensure the bot is working smoothly. Overall this is looking pretty good. Once the output is pasted ill review again. Ill keep an eye out. Thank you |
|
Hey @aceppaluni and @cheese-cakee |
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-beginner-assign-on-comment.js (1)
279-295: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider removing the early already-assigned check to ensure all decisions use fresh issue state.
This check reads
issue.assigneesfrom the webhook payload, which can be stale for queued workflow runs. Lines 377-393 perform the definitive already-assigned check using the freshly refetchedcurrentIssue. The early check here is redundant and creates a potential false-negative edge case: if the payload is stale in the opposite direction (shows assigned when the issue is actually unassigned), this early exit would incorrectly block assignment.Removing this block would align with the stated objective of "verify current issue state from the GitHub API before the final assignment decision" and match the consistency recommendation from cheese-cakee's feedback.
♻️ Proposed removal
- // --- ASSIGNMENT LOGIC --- - if (issue.assignees && issue.assignees.length > 0) { - try{ - await github.rest.issues.createComment({ - owner: repo.owner.login, - repo: repo.name, - issue_number: issue.number, - body: buildAlreadyAssignedComment({ commenter, issue, repo }), - }); - } catch (error) { - console.error("[Beginner Bot] Failed to post already-assigned message:", { - issue: issue.number, - commenter, - message: error.message, - }); - } - return; - } -.github/scripts/bot-gfi-assign-on-comment.js (1)
323-335: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider removing the early already-assigned check to ensure all decisions use fresh issue state.
This check reads
issue.assigneesfrom the webhook payload, which can be stale for queued workflow runs. Lines 409-421 perform the definitive already-assigned check using the freshly refetchedcurrentIssue. The early check here is redundant and creates a potential false-negative edge case: if the payload is stale in the opposite direction (shows assigned when the issue is actually unassigned), this early exit would incorrectly block assignment.Removing this block would align with the stated objective of "verify current issue state from the GitHub API before the final assignment decision" and match the consistency recommendation from cheese-cakee's feedback.
♻️ Proposed removal
- // Reject if issue is already assigned - // Comment failure to the requester - if (issue.assignees?.length > 0) { - console.log('[gfi-assign] Exit: issue already assigned'); - - await github.rest.issues.createComment({ - owner, - repo, - issue_number: issueNumber, - body: commentAlreadyAssigned(requesterUsername, issue), - }); - - console.log('[gfi-assign] Posted already-assigned comment'); - return; - } -
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: faa1afde-e3f0-4fdb-924a-be2518f8b842
📒 Files selected for processing (3)
.github/scripts/bot-assignment-fresh-issue.test.js.github/scripts/bot-beginner-assign-on-comment.js.github/scripts/bot-gfi-assign-on-comment.js
| 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; | ||
| } |
There was a problem hiding this comment.
🧹 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;
}
|
@chaitanyamedidar No problem, will mark this as draft for now. Good luck with your exams!! |
Description:
This PR updates the GFI and beginner assignment bots to refetch the current GitHub issue state immediately before calling
addAssignees.This avoids making the final assignment decision from a stale
issue_comment.createdwebhook payload when another queued workflow run may have already assigned the issue.Changes:
Related issue(s):
Fixes #2230
Notes for reviewer:
This intentionally keeps the Python fix small. It follows the same safety pattern used in the C++ assignment workflow: check the payload first, then refetch current issue state before the final write. I did not port the broader C++ workflow structure in this PR.
Checklist