[ci:change] Stale-PR bot: drop auto-close and fix false-positive detection#668
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refines StalePRBot's staleness detection: it adds _commit_activity_date_for_author to derive a per-commit activity timestamp from author and committer dates and updates _get_last_author_activity to use it; get_last_changes_requested now collapses reviews to each reviewer's latest applicable review (skipping bots, missing fields, and COMMENTED) and returns the newest CHANGES_REQUESTED among them. It replaces auto-closing at 60 days with send_final_followup that posts a final-followup comment, and updates tests and docs to cover committer fallback, force-push, bot-review cases, dismissal, and threshold behavior. Sequence Diagram(s)sequenceDiagram
participant Scheduler as process_stale_prs
participant GitHub as GitHub API
participant Bot as StalePRBot
Scheduler->>GitHub: fetch PRs, commits, reviews, comments
Scheduler->>Bot: evaluate PR (get_days_since_activity, get_last_changes_requested)
Bot->>Bot: compute last author activity from commits/reviews
Bot->>Bot: determine days_inactive and waiting_for_maintainer
alt maintainer waiting detected
Bot->>GitHub: _clear_stale_label
Bot->>GitHub: _reassign_unassigned_linked_issues
else not waiting
alt days_inactive >= DAYS_BEFORE_FINAL_FOLLOWUP
Bot->>GitHub: send_final_followup (post final-followup comment)
else handle earlier warning/stale labeling
Bot->>GitHub: post warnings / mark_pr_stale (label + unassign)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ 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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental review completed for commit Changes Reviewed
Verification
Files Reviewed (2 files)
Incremental review by kimi-k2.5 · Previous: 9a3f8d0 Reviewed by kimi-k2.5 · 263,683 tokens |
There was a problem hiding this comment.
Pull request overview
This PR updates the stale PR management GitHub Action so that PRs are not treated as “blocked by changes requested” when the only blocking reviews come from bots, preventing automated stale-PR workflows from closing PRs that aren’t actually awaiting contributor action.
Changes:
- Update
get_last_changes_requestedto consider only the latest review per human reviewer (ignoring bots and non-blocking “COMMENTED” reviews). - Improve author activity detection by considering committer metadata/dates in addition to author metadata/dates.
- Add/extend tests to cover bot-review blocking scenarios and committer-vs-author commit activity cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| .github/actions/bot-autoassign/stale_pr_bot.py | Adjusts review-blocking detection to ignore bot reviews and uses author/committer metadata when computing contributor activity. |
| .github/actions/bot-autoassign/tests/test_stale_pr_bot.py | Adds test coverage for bot review blocking behavior and committer-date based activity detection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if author_date and committer_date: | ||
| commit_date = max(author_date, committer_date) | ||
| else: | ||
| commit_date = author_date or committer_date | ||
| if not commit_date or commit_date <= after_date: | ||
| continue | ||
| author_login = commit.author.login if commit.author else None | ||
| committer_login = commit.committer.login if commit.committer else None | ||
| if author_login == pr_author or committer_login == pr_author: | ||
| if not last_activity or commit_date > last_activity: | ||
| last_activity = commit_date |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/bot-autoassign/stale_pr_bot.py:
- Around line 35-49: The code currently takes commit_date = max(author_date,
committer_date) before checking which identity matches pr_author, allowing
someone else's later committer timestamp to advance last_activity; change the
logic so you first compute author_login and committer_login (variables present)
and then set commit_date based on which identity matches pr_author: if both
author_login == pr_author and committer_login == pr_author use max(author_date,
committer_date); else if only author_login == pr_author use author_date; else if
only committer_login == pr_author use committer_date; otherwise fall back to the
existing author_date or committer_date logic; finally use that commit_date when
comparing to after_date and updating last_activity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8971cc46-63b1-487b-a547-38ef9313e192
📒 Files selected for processing (2)
.github/actions/bot-autoassign/stale_pr_bot.py.github/actions/bot-autoassign/tests/test_stale_pr_bot.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Agent
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Kilo Code Review
🔇 Additional comments (1)
.github/actions/bot-autoassign/tests/test_stale_pr_bot.py (1)
607-654: Good regression coverage for the bot-review false-positive cases.These scenarios lock in the exact behavior this PR is trying to fix: bot-only
CHANGES_REQUESTEDreviews no longer start the stale flow, and later human approvals clear earlier human blocks.
pandafy
left a comment
There was a problem hiding this comment.
@Eeshu-Yadav I think we can make the code more readable which will reduce maintenance efforts in the long run.
| print("Error checking maintainer activity" f" for PR #{pr.number}: {e}") | ||
| return False | ||
|
|
||
| def get_last_changes_requested(self, pr, all_reviews=None): |
There was a problem hiding this comment.
Add a docstring like other methods.
| changes_requested_reviews = [ | ||
| r for r in all_reviews if r.state == "CHANGES_REQUESTED" | ||
| latest_per_reviewer = {} | ||
| for r in all_reviews: |
There was a problem hiding this comment.
Don't use one letter variable names.
There was a problem hiding this comment.
Done - r → review.
| for r in all_reviews: | ||
| if not r.user or not r.submitted_at: | ||
| continue | ||
| if r.user.type == "Bot": | ||
| continue | ||
| if r.state == "COMMENTED": | ||
| continue | ||
| current = latest_per_reviewer.get(r.user.login) | ||
| if current is None or r.submitted_at > current.submitted_at: | ||
| latest_per_reviewer[r.user.login] = r | ||
| blocking = [ | ||
| r | ||
| for r in latest_per_reviewer.values() | ||
| if r.state == "CHANGES_REQUESTED" | ||
| ] |
There was a problem hiding this comment.
Add an inline comment explaining why are are skipping review.type == "Bot" or review.state == "COMMENTED".
What is r.user.login and why do we need that?
Add an inline comment for the entire loop to explain what it does (shortly) and emphasize on WHY.
| @@ -32,11 +32,21 @@ | |||
| return None | |||
| last_activity = None | |||
| for commit in pr.get_commits(): | |||
There was a problem hiding this comment.
Does pr.get_commits() return commits in descending order? i.e. the newest commit should be the first in the list to minimize the execution.
There was a problem hiding this comment.
Commits come oldest-first, not newest-first. Tried optimizing with reverse-iterate but reverted , when rebases mix up author vs committer dates, breaking early can grab the wrong commit.
| author_date = commit.commit.author.date if commit.commit.author else None | ||
| committer_date = ( | ||
| commit.commit.committer.date if commit.commit.committer else None | ||
| ) | ||
| if author_date and committer_date: | ||
| commit_date = max(author_date, committer_date) | ||
| else: | ||
| commit_date = author_date or committer_date | ||
| if not commit_date or commit_date <= after_date: | ||
| continue | ||
| author_login = commit.author.login if commit.author else None | ||
| committer_login = commit.committer.login if commit.committer else None | ||
| if author_login == pr_author or committer_login == pr_author: | ||
| if not last_activity or commit_date > last_activity: | ||
| last_activity = commit_date |
There was a problem hiding this comment.
This logic can be moved to a private helper method. It will make code more readable.
There was a problem hiding this comment.
Done - extracted to _commit_activity_date_for_author.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/bot-autoassign/stale_pr_bot.py:
- Around line 420-429: The current ordering jumps straight to
send_final_followup when days_inactive >= DAYS_BEFORE_FINAL_FOLLOWUP and skips
the intermediate stale/unassign logic; reorder the conditional checks so the
DAYS_BEFORE_UNASSIGN (14-day stale/unassign stage) is evaluated before
DAYS_BEFORE_FINAL_FOLLOWUP (60-day) and still allows send_final_followup to run
afterwards; update the block around has_bot_comment, send_final_followup,
DAYS_BEFORE_UNASSIGN and DAYS_BEFORE_FINAL_FOLLOWUP to first handle adding the
stale label/unassigning (and use has_bot_comment to avoid duplicate comments)
when days_inactive >= DAYS_BEFORE_UNASSIGN, then run the final followup branch
when days_inactive >= DAYS_BEFORE_FINAL_FOLLOWUP; add a regression test that
simulates a PR first processed at >60 days to assert the stale label/unassign
happened and the final followup was sent.
In `@docs/developer/reusable-github-utils.rst`:
- Around line 97-100: The sentence "Pushing commits or replying revives the PR"
is misleading because only issue_comment and reopened events route through the
recovery bot; a plain push only resets the inactivity timer but does not remove
the stale label or reassign linked issues. Update the bullet text to say
something like: "Pushing commits resets the inactivity clock, but only
issue_comment or reopened events will route through the recovery bot to remove
the ``stale`` label and reassign linked issues; replying or reopening the PR
triggers recovery." Reference the existing phrase "Pushing commits or replying
revives the PR", the ``stale`` label, and the events ``issue_comment`` and
``reopened`` when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3cdb2cb-0cdd-4dc5-b175-18d7af602ab2
📒 Files selected for processing (3)
.github/actions/bot-autoassign/stale_pr_bot.py.github/actions/bot-autoassign/tests/test_stale_pr_bot.pydocs/developer/reusable-github-utils.rst
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
2ce0ae8 to
bbc01e6
Compare
| the contributor's commits does not reset the clock. | ||
| 3. **Maintainer-court skip.** If a maintainer (``OWNER``, ``MEMBER`` or | ||
| ``COLLABORATOR``) has commented or reviewed after the contributor's | ||
| last action, the PR is skipped — the ball is in the maintainer's court. |
There was a problem hiding this comment.
This seems wrong. I think it should be the opposite shouldn't it?
If no maintainer has reviewed, then the ball is in the maintainer's court.
Moreover, we should only target reviews, not comments, as we can comment to ask follow up questions but that doesn't count as a review.
Approach is correct but wider than the issue describesThe reported bug is that PRs waiting for review are being auto-closed. The PR addresses that with three independent changes:
Each of (1) and (2) on their own solves the reported issue. (3) is a separate scope decision — "we no longer auto-close anything." That's defensible (it eliminates the worst-case mis-action), but the PR title still says "stop … from closing PRs blocked only by bot reviews," which understates the scope. Worth tightening the PR title and description so reviewers understand auto-close is gone, period. Stale label gets stuck and never clears once appliedThis is the most user-visible regression introduced by removing auto-close. Two paths reach the same dead end: Author pushes a fix without commenting.
The bot reaches stage 3 and then has nothing more to do. Once Two cheap fixes:
A first-time-processed PR over 60 days old gets two comments at once
For PRs the bot tracked continuously this can't happen; the marker check filters past stages. But for a PR the bot missed (workflow disabled, repo migration, schedule outage), it produces a noisy resurfacing. Gating stage 3 on the existence of stage 2's marker would make the follow-up only fire after the stale notification was posted in a previous run, mirroring the natural progression.
|
bbc01e6 to
44b2d2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/bot-autoassign/stale_pr_bot.py:
- Around line 425-433: Add push-driven stale-state cleanup by wiring the
pull_request.synchronize event to dispatch into PRActivityBot so it invokes
_clear_stale_label(pr) and _reassign_unassigned_linked_issues(pr) immediately
when a contributor pushes; keep the existing cron as a fallback. Concretely, add
a handler for pull_request (action "synchronize") or pull_request_target that
loads the same PRActivityBot logic used by issue_comment and calls the existing
methods _clear_stale_label and _reassign_unassigned_linked_issues with the PR
object so the stale label is cleared and linked issues are reassigned on push.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f865815-3113-4c2f-bbb6-c17882bb5855
📒 Files selected for processing (3)
.github/actions/bot-autoassign/stale_pr_bot.py.github/actions/bot-autoassign/tests/test_stale_pr_bot.pydocs/developer/reusable-github-utils.rst
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.0.0
🔇 Additional comments (1)
.github/actions/bot-autoassign/stale_pr_bot.py (1)
164-194: LGTM — clean per-reviewer "current stance" computation.The collapse of reviews to each reviewer's latest non-COMMENTED stance, with bot reviews and missing-field reviews filtered out before the CHANGES_REQUESTED check, is a solid correctness fix for the false-positive cases (bot reviews, superseded blocks, dismissals). The test coverage in
TestGetLastChangesRequestedexercises each path well.
|
The latest commit (44b2d2a) addresses most of the prior review feedback. The following items remain. Doc claim about push-vs-comment is now wrong
The new
Stale label can still get stuck if the blocking review is later approved/dismissed
if not last_changes_requested:
continueScenario: PR marked stale at day 14. At day 20 the reviewer approves (or dismisses their own review). On the next daily run, Cheap fix: hoist the Partial-unassign produces a false comment
Two-line
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/bot-autoassign/stale_pr_bot.py:
- Around line 21-22: Normalize the BOT_USERNAME before appending "[bot]" to
avoid double-suffixes: update the assignment to self.bot_login so it reads the
BOT_USERNAME env var, strip any trailing "[bot]" (e.g., using rstrip or
conditional remove) and then append a single "[bot]"; reference the BOT_USERNAME
env fetch and the self.bot_login variable to locate the change and ensure
existing callers without the suffix remain unaffected and has_bot_comment
matching still works.
- Around line 192-194: The current except in get_last_changes_requested swallows
exceptions from pr.get_reviews() and returns None, which the caller
process_stale_prs treats as “no blocking review” and clears the stale label;
change get_last_changes_requested to not fail-open: either re-raise the
exception or return an explicit sentinel (e.g., raise the exception or return a
distinct value like "ERROR") instead of None, and then update process_stale_prs
to detect that error sentinel (or catch the propagated exception) and
continue/skip processing that PR rather than calling _clear_stale_label and
_reassign_unassigned_linked_issues; adjust surrounding logic in
get_last_changes_requested and process_stale_prs to handle the new contract.
In @.github/actions/bot-autoassign/tests/test_stale_pr_bot.py:
- Around line 685-713: Add a companion regression test that simulates the
two-run sequence: instantiate StalePRBot, create mock_pr with the same
review/commit/comment/label setup used in
test_pr_first_processed_past_60_days_marks_stale_only, run
bot.process_stale_prs() once and assert the presence of "<!-- bot:stale -->" and
that mock_pr.add_to_labels("stale") was called and mock_pr.edit was not called,
then run bot.process_stale_prs() a second time and assert the presence of "<!--
bot:final_followup -->" (and that no duplicate "<!-- bot:stale -->" or "<!--
bot:stale_warning -->" comments are posted), plus verify add_to_labels is not
called again and mock_pr.edit remains not called; use the same mocks (mock_pr,
cr_review) and inspect mock_pr.create_issue_comment.call_args_list to check
comment bodies to ensure the bot transitions from stale to final_followup on the
second run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 99eade9b-a0b3-4d5f-8f53-7190b0740cfd
📒 Files selected for processing (3)
.github/actions/bot-autoassign/stale_pr_bot.py.github/actions/bot-autoassign/tests/test_stale_pr_bot.pydocs/developer/reusable-github-utils.rst
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Kilo Code Review
🔇 Additional comments (3)
.github/actions/bot-autoassign/tests/test_stale_pr_bot.py (1)
46-46: LGTM!Also applies to: 80-217, 233-323, 344-469, 507-545, 580-600, 612-683, 715-782
docs/developer/reusable-github-utils.rst (1)
72-106: LGTM!.github/actions/bot-autoassign/stale_pr_bot.py (1)
24-41: LGTM!Also applies to: 47-47, 60-67, 98-98, 109-109, 118-118, 125-125, 130-131, 136-136, 140-143, 161-162, 165-167, 171-191, 197-198, 207-207, 215-215, 219-226, 228-254, 260-261, 265-266, 270-272, 275-276, 279-279, 299-301, 321-321, 327-327, 332-332, 400-402, 406-406, 410-410, 416-416, 421-421, 426-432, 434-473, 475-475, 479-479, 487-487
94003e2 to
9a3f8d0
Compare
Checklist
Reference to Existing Issue
Fixes the stale-PR bot incident where ~16 PRs across
openwisp-utils,openwisp-controller, andopenwisp-monitoringwere auto-closed after the bot mistook bot reviews (coderabbitai) for blocking maintainer reviews. Federico's temporary stub on master disabled auto-close while this proper fix was prepared.What changes
Detection (the actual bug)
get_last_changes_requestednow filters out bot reviews and tracks each reviewer's latest non-COMMENTED state — a reviewer who later approves or dismisses no longer blocks._commit_activity_date_for_authorpicks the date of whichever identity (author or committer) actually matches the PR author, so a maintainer rebasing the contributor's commits is no longer counted as contributor activity.Auto-close removed
close_stale_pris gone. At 60 days the bot now posts afinal_followupencouragement comment and leaves the PR open. Maintainers can close manually if a PR is genuinely abandoned.Stale-state recovery
last_changes_requested, the bot clears thestalelabel and reassigns linked issues that are currently unassigned. Previously the label and issue state could persist indefinitely.Tighter
is_waiting_for_maintainerOther
has_bot_commentmatches our bot's identity so a third-party bot reusing the marker can't suppress our stages.pr.get_commits()is materialized once per PR and threaded through, removing duplicate API iteration.mark_pr_staleunassigns issues before posting the "unassigned" comment, so the comment can't claim something that didn't happen.Testing on
bot-testing-ground