Repository navigation
fix: refetch current issue state before assignment bots assign users - #2237
chaitanyamedidar wants to merge 3 commits into
Conversation
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.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Layer / File(s) | Summary |
|---|---|
Fetch issue from GitHub .github/scripts/shared/api/github-api.js |
Adds and exports getIssue, which fetches an issue by owner, repository, and issue number and returns the API response data. |
Gate assignment on refreshed issue .github/scripts/shared/core/issue-assign.js, .github/scripts/__tests__/jest/issue-assign.test.js |
The assignment flow checks the fetched issue before assigning and uses it for the already-assigned notice and CodeRabbit plan trigger. Tests cover fetch failure, changed skill label, closed state, and existing assignees. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Severity of issue fixed: Medium
Sequence Diagram(s)
sequenceDiagram
participant AssignmentFlow
participant GitHubAPI
participant CodeRabbit
AssignmentFlow->>GitHubAPI: Fetch current issue
GitHubAPI-->>AssignmentFlow: Return issue state, labels, and assignees
alt Issue is eligible and unassigned
AssignmentFlow->>GitHubAPI: Assign issue
AssignmentFlow->>CodeRabbit: Trigger plan with refreshed issue
else Issue is ineligible or already assigned
AssignmentFlow->>AssignmentFlow: Stop assignment flow
end
Merge Risk: 
2c3ef
-
The change is mergeable. An existing early check can still reject an issue after its assignee is removed.
Pre-merge checks: 
-
Check name Status Explanation Title check The title names the main change: the assignment bots now refetch the current issue state before assigning users. It follows the conventional commit style, is concise, and matches the summary of change… Description check The description states that the GFI and beginner assignment bots now refetch current issue state before calling addAssignees. It lists the changes, including fail-safe handling, label and assignee rec… Linked Issues check Issue #2230requires both assignment bots to use current assignees, labels, and issue state before the final assignment. The sharedrunAssignmentFlownow callsgetIssueafter the prerequisite and …Out of Scope Changes check The changes stay within issue #2230. The addedgetIssueAPI helper supports the required refetch. The shared assignment-flow change applies the behavior to the GFI and beginner bots. The updated Jes…Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Full details: Title check
-
Explanation
The title names the main change: the assignment bots now refetch the current issue state before assigning users. It follows the conventional commit style, is concise, and matches the summary of changes and objectives.
Full details: Description check
-
Explanation
The description states that the GFI and beginner assignment bots now refetch current issue state before calling addAssignees. It lists the changes, including fail-safe handling, label and assignee rechecks, and a regression test. This matches the changeset.
Full details: Linked Issues check
-
Explanation
Issue
#2230requires both assignment bots to use current assignees, labels, and issue state before the final assignment. The sharedrunAssignmentFlownow callsgetIssueafter the prerequisite and limit checks, stops when the refreshed issue is not open, no longer has the required skill label, or has assignees, and callsassignIssueonly after those checks. The fetch failure path returns without assignment. The Jest tests cover assigned, unavailable, missing-label, and closed refreshed issues.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within issue
#2230. The addedgetIssueAPI helper supports the required refetch. The shared assignment-flow change applies the behavior to the GFI and beginner bots. The updated Jest tests verify the required safety paths and assignment behavior. No unrelated change is shown in the supplied pull request evidence.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 3 files.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![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, |
exploreriii
added
step: 1st
|
LGTM ! Everything looks fine to me. Just few notes we should consider for future polish : Expanded Test Coverage
|
|
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
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
|
@chaitanyamedidar No problem, will mark this as draft for now. Good luck with your exams!! |
|
This pull request has had no commit activity for 10 days. Are you still working on it?
If you're no longer working on this, please comment Reach out on discord or join our office hours if you need assistance. From the Python SDK Team |
|
This pull request has had no activity for 10 days. Are you still working on it?
If you're no longer working on this, please comment Reach out on discord or join our office hours if you need assistance. From the Python SDK Team |
|
@chaitanyamedidar Are you still planning to work on this? |
|
Hello @aceppaluni, sorry for the delayed response. Yes, I intend to work on it. However, it could take a while. If this work is required to be completed urgently, I'd kindly request one of the members to take on this work. |
|
@chaitanyamedidar No problem, I wanted to make sure our bots did not close this on you if you had been working. Thank you for letting us know! |
|
This pull request has had no activity for 10 days. Are you still working on it?
If you're no longer working on this, please comment Reach out on discord or join our office hours if you need assistance. From the Python SDK Team |
|
/working |
|
@chaitanyamedidar If you are still working on this please comment /working only on the issue as it is not counted when commented in the PR. If you have any questions please let me know. Thank you! |
|
@aceppaluni Noted. |
|
This pull request has had no activity for 10 days. Are you still working on it?
If you're no longer working on this, please comment Reach out on discord or join our office hours if you need assistance. From the Python SDK Team |
1 similar comment
|
This pull request has had no activity for 10 days. Are you still working on it?
If you're no longer working on this, please comment Reach out on discord or join our office hours if you need assistance. From the Python SDK Team |
|
@chaitanyamedidar Are you still working on this? |
d0c2af8 to
9a5fd1a
Compare
Signed-off-by: chaitanyamedidar <175808937+chaitanyamedidar@users.noreply.github.com>
|
@aceppaluni @exploreriii PR is up for review, thank you for being patient here! |
|
This pull request has had no activity for 10 days. Are you still working on it?
If you're no longer working on this, please comment Reach out on discord or join our office hours if you need assistance. From the Python SDK Team |
|
This pull request has had no activity for 10 days. Are you still working on it?
If you're no longer working on this, please comment Reach out on discord or join our office hours if you need assistance. From the Python SDK Team |
|
@chaitanyamedidar Are you still working on this? |
|
@chaitanyamedidar Are you still working on this? If not no problem, we will close this PR. Thank you. |
|
Hi @aceppaluni, I've made the changes at request. I'd appreciate your and others review on the same. Thanks!
|
|
@chaitanyamedidar can you update your branch when you have a moment please? Thank you |
|
@aceppaluni done. |
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- Source mode: Auto
- Repository: hiero-ledger/hiero-sdk-python
- Branch: main
- File: .coderabbit.yaml
- Commit: c9336f1
- Layers: Repository YAML
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
b11ac3d1-8b4b-4de0-857f-6da4a55e7983
-
.github/scripts/__tests__/jest/issue-assign.test.js.github/scripts/shared/api/github-api.js.github/scripts/shared/core/issue-assign.js
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
-
| // /assign runs, but their webhook payload stays stale. | ||
| let currentIssue; | ||
| try { | ||
| currentIssue = await getIssue({ github, owner, repo: repoName, issueNumber }); |
There was a problem hiding this comment.
Check the current issue before rejecting an assigned webhook issue.
If an assignee is removed while a /assign run waits, the webhook issue can still show that assignee. The existing isAssigned branch at Lines 198–203 then posts an incorrect already-assigned notice and returns before this fetch. Check current state before that rejection, and retain a fetch immediately before the assignment write. (raw.githubusercontent.com)
github-actions
Bot
added
open to community review
|
This pull request has had no activity for 10 days. Are you still working on it?
If you're no longer working on this, please comment Reach out on discord or join our office hours if you need assistance. From the Python SDK Team |
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