Skip to content

[ci:change] Stale-PR bot: drop auto-close and fix false-positive detection#668

Merged
nemesifier merged 9 commits into
openwisp:masterfrom
Eeshu-Yadav:fix/stale-pr-bot-bot-reviewer-false-positives
May 13, 2026
Merged

[ci:change] Stale-PR bot: drop auto-close and fix false-positive detection#668
nemesifier merged 9 commits into
openwisp:masterfrom
Eeshu-Yadav:fix/stale-pr-bot-bot-reviewer-false-positives

Conversation

@Eeshu-Yadav
Copy link
Copy Markdown
Contributor

@Eeshu-Yadav Eeshu-Yadav commented May 8, 2026

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

Reference to Existing Issue

Fixes the stale-PR bot incident where ~16 PRs across openwisp-utils, openwisp-controller, and openwisp-monitoring were 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_requested now 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_author picks 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_pr is gone. At 60 days the bot now posts a final_followup encouragement comment and leaves the PR open. Maintainers can close manually if a PR is genuinely abandoned.

Stale-state recovery

  • When the cron sees contributor activity past last_changes_requested, the bot clears the stale label and reassigns linked issues that are currently unassigned. Previously the label and issue state could persist indefinitely.

Tighter is_waiting_for_maintainer

  • Only counts maintainer reviews, not comments — a follow-up question in a comment doesn't hand the ball back to the contributor.
  • Fail-closed: on any exception, the bot now skips the PR instead of proceeding.

Other

  • has_bot_comment matches 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_stale unassigns issues before posting the "unassigned" comment, so the comment can't claim something that didn't happen.
  • Final-followup waits a run after the stale notification so the two comments don't land back-to-back.

Testing on bot-testing-ground

Scenario Reference
Bot-only CR ignored bot-testing-ground#103
Stale at 14 days (label + unassign) bot-testing-ground#102
Stale-label recovery on contributor push bot-testing-ground#102
Day-60 final follow-up, PR stays open bot-testing-ground#102
Supersession by APPROVED bot-testing-ground#104
Supersession by DISMISSED bot-testing-ground#105
Maintainer comment doesn't reset bot-testing-ground#106
Third-party marker not trusted bot-testing-ground#107
First-time PR past 60 days bot-testing-ground#108
Maintainer rebase doesn't reset clock bot-testing-ground#109

Copilot AI review requested due to automatic review settings May 8, 2026 17:32
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 8, 2026

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This 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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • nemesifier

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error)

Check name Status Explanation Resolution
Bug Fixes ❌ Error Core bug fixes (bot filtering, maintainer rebases) have adequate regression tests. Missing two-run sequence test per review comment lines 685-713. Add test calling process_stale_prs() twice: first run posts stale at 60+ days, second run posts final_followup without duplicating stale.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description is comprehensive and well-structured, covering all required checklist items, the issue being fixed, detailed changes across multiple areas, and testing validation with references.
Title check ✅ Passed The title includes the required [ci:change] prefix, is descriptive, and accurately reflects the main changes: removing auto-close behavior and fixing false-positive detection from bot reviews.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kilo-code-bot
Copy link
Copy Markdown

kilo-code-bot Bot commented May 8, 2026

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Incremental review completed for commit 2610bfb. All changes since the previous review (9a3f8d0) have been verified.

Changes Reviewed

  1. stale_pr_bot.py - _clear_stale_label optimization (line 230-239):

    • Changed from pr.get_labels() to pr.labels to avoid extra API request
    • pr.labels is populated from the list-pulls payload in PyGithub
    • Added explanatory comment clarifying this behavior
  2. stale_pr_bot.py - posted_stale_this_run flag ordering fix (lines 473-479):

    • Critical fix: Flag is now set BEFORE attempting the stale action, not after success
    • Prevents final_followup from posting in the same run if mark_pr_stale fails
    • Ensures contributors don't receive out-of-context follow-up without prior stale notice
    • Comprehensive inline comment added explaining the rationale
  3. test_stale_pr_bot.py - New test coverage:

    • test_propagates_exception: Verifies get_last_changes_requested propagates exceptions
    • test_first_run_past_60_days_failed_stale_skips_followup: Validates the flag ordering fix
    • Updated existing tests to use mock_pr.labels attribute

Verification

  • ✅ API optimization is valid PyGithub behavior
  • ✅ Flag ordering fix prevents race condition between stale and final-followup stages
  • ✅ Test coverage added for new edge cases
  • ✅ No breaking changes to existing functionality
  • ✅ All previous review issues addressed
Files Reviewed (2 files)
  • .github/actions/bot-autoassign/stale_pr_bot.py - Label optimization, flag ordering fix
  • .github/actions/bot-autoassign/tests/test_stale_pr_bot.py - New test coverage

Incremental review by kimi-k2.5 · Previous: 9a3f8d0


Reviewed by kimi-k2.5 · 263,683 tokens

@coderabbitai coderabbitai Bot added bug github_actions Pull requests that update GitHub Actions code helper-bots Helper bots, release management automation labels May 8, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_requested to 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.

Comment on lines +39 to +49
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
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 0e66d19 and 2468f85.

📒 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_REQUESTED reviews no longer start the stale flow, and later human approvals clear earlier human blocks.

Comment thread .github/actions/bot-autoassign/stale_pr_bot.py Outdated
@coveralls
Copy link
Copy Markdown

coveralls commented May 8, 2026

Coverage Status

coverage: 97.514%. remained the same — Eeshu-Yadav:fix/stale-pr-bot-bot-reviewer-false-positives into openwisp:master

Copy link
Copy Markdown
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a docstring like other methods.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

changes_requested_reviews = [
r for r in all_reviews if r.state == "CHANGES_REQUESTED"
latest_per_reviewer = {}
for r in all_reviews:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use one letter variable names.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - rreview.

Comment on lines 174 to 188
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"
]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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():
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +35 to +49
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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic can be moved to a private helper method. It will make code more readable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - extracted to _commit_activity_date_for_author.

coderabbitai[bot]
coderabbitai Bot previously approved these changes May 8, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 98ba0a6 and 2ce0ae8.

📒 Files selected for processing (3)
  • .github/actions/bot-autoassign/stale_pr_bot.py
  • .github/actions/bot-autoassign/tests/test_stale_pr_bot.py
  • docs/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

Comment thread .github/actions/bot-autoassign/stale_pr_bot.py Outdated
Comment thread docs/developer/reusable-github-utils.rst
@Eeshu-Yadav Eeshu-Yadav force-pushed the fix/stale-pr-bot-bot-reviewer-false-positives branch from 2ce0ae8 to bbc01e6 Compare May 9, 2026 12:26
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.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nemesifier
Copy link
Copy Markdown
Member

nemesifier commented May 9, 2026

Approach is correct but wider than the issue describes

The reported bug is that PRs waiting for review are being auto-closed. The PR addresses that with three independent changes:

  1. get_last_changes_requested now ignores Bot reviews and uses each reviewer's latest non-COMMENTED state — so a coderabbitai[bot] "request changes" no longer marks a PR as blocked, and a human who later approved or dismissed their own review no longer counts.
  2. _commit_activity_date_for_author now considers both commit.author and commit.committer, so a maintainer rebasing the contributor's commits doesn't reset the inactivity clock; the contributor's force-push, where both identities match, takes the committer date (always later).
  3. close_stale_pr is removed entirely; the 60-day stage now posts a final_followup comment instead of closing.

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 applied

This is the most user-visible regression introduced by removing auto-close. Two paths reach the same dead end:

Author pushes a fix without commenting. mark_pr_stale adds the stale label and unassigns linked issues at day 14. The author pushes a commit on day 16. On the next daily run, is_waiting_for_maintainer returns True (the new commit is the latest activity), so process_stale_prs skips the PR. Nothing in stale_pr_bot.py removes the label.

PRActivityBot.handle_contributor_activity (pr_reopen_bot.py:115) is the code that would clear the label and re-assign issues, but its workflow only triggers on issue_comment.created and pull_request_target.reopened (bot-autoassign-pr-reopen.yml:4-7). There's no handler for pull_request.synchronize. A contributor who pushes without commenting stays in the stale state.

The bot reaches stage 3 and then has nothing more to do. Once bot:stale and bot:final_followup markers exist with created_at > last_changes_requested, every future run skips both stages. The stale label and the unassigned-issues state persist until a maintainer manually closes the PR or the author comments to trigger PRActivityBot. Previously auto-close cleaned this up at 60 days; now nothing does.

Two cheap fixes:

  • Have process_stale_prs clear the stale label and re-assign issues whenever it observes new author commits past last_changes_requested, or
  • Trigger PRActivityBot (or an equivalent) on pull_request.synchronize so a push counts the same as a comment.

A first-time-processed PR over 60 days old gets two comments at once

test_pr_first_processed_past_60_days_marks_stale_and_followup enshrines the behavior: with last_changes_requested on Feb 1 and "now" on May 10 (≈99 days), stage 1 is skipped (high gate=14), stage 2 fires (mark_pr_stale), and stage 3 fires (send_final_followup) — all in the same run. The contributor receives two bot comments back-to-back, plus a label change and an issue unassignment.

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.

is_waiting_for_maintainer fail-opens on API errors

except Exception as e:
    print("Error checking maintainer activity" f" for PR #{pr.number}: {e}")
    return False

False here means "go ahead and process the PR." So a transient PyGithub or rate-limit error makes the bot more aggressive on a PR it can't fully assess. Now that auto-close is gone the worst case is a wrong stale-warning or stale-label, which is recoverable, but the policy should still be: when in doubt, skip. Default to True on exception.

get_days_since_activity (returns 0) and get_last_changes_requested (returns None) have the same fail-open shape — both fall through to "process the PR" — though their blast radius is smaller.

has_bot_comment matches any bot's comment containing the marker

if (
    comment.user
    and comment.user.type == "Bot"
    and marker in comment.body
):

Any bot whose comment body contains <!-- bot:stale --> (quoted excerpt, fork's bot reusing the convention, etc.) satisfies the gate, and the corresponding stage is silently skipped. Realistic risk is low — the markers are unique to this bot — but the fix is one line: also match the comment author against the workflow's bot identity (github-actions[bot]). Pre-existing, but the PR adds a third marker (final_followup) and increases the surface area.

pr.get_commits() is iterated in full on every check

_get_last_author_activity calls pr.get_commits() and iterates the entire paginated result. PyGithub paginates lazily, so for a long-running PR with hundreds of commits each daily run pulls every page just to find the latest one whose author or committer matches pr_author. Cost scales with PR commit count × stale-eligible PRs × daily runs.

Worse, process_stale_prs calls _get_last_author_activity twice per PR — once via get_days_since_activity and once via is_waiting_for_maintainer — and each call re-iterates get_commits() from scratch because (unlike issue_comments, review_comments, and all_reviews) it isn't cached at the per-PR scope. Hoist the commit list to process_stale_prs and pass it through like the other three.

mark_pr_stale posts the unassign comment before actually unassigning

pr.create_issue_comment("\n".join(unassign_lines))   # claims issues are unassigned
unassigned_count = self.unassign_linked_issues(pr)   # actually unassigns them

If the comment-post succeeds but unassign_linked_issues fails (rate limit, permission, etc.), the contributor reads "the linked issue(s) have been unassigned from you" while the issue is still assigned to them. Reorder, or word the comment as "we are unassigning…" instead of "have been unassigned." Pre-existing.

send_final_followup opens with bare Hi @author, — others have 👋

mark_pr_stale and send_stale_warning open with Hi @{author} 👋,. send_final_followup opens with Hi @{author},. Cosmetic, but reads as if the 60-day comment is deliberately colder. Standardize.

Two-line print("..." f" ...") concatenations

Pattern repeats across the file (print("Error calculating activity" f" for PR #{pr.number}: {e}"), print("Error checking maintainer activity" f" for PR #{pr.number}: {e}"), etc.). Python concatenates adjacent literals at compile time so it's functional, but the result reads like an accidental tuple and the f-string has a leading space that only makes sense alongside its neighbor. Pre-existing, but the new code copies it verbatim — collapse to single f-strings while you're touching the file.

Test coverage gaps

  • No test for the "PR was stale, author pushed, label not removed" scenario described above. Add one as either a regression test (if you fix the behavior) or as an explicit pin of the current behavior so future contributors don't "fix" it accidentally.
  • No test for is_waiting_for_maintainer when one of the three GitHub API calls inside it raises. Given the fail-open concern above, this branch deserves coverage.

@Eeshu-Yadav Eeshu-Yadav changed the title [fix] Stop stale-PR bot from closing PRs blocked only by bot reviews [change] Stale-PR bot: drop auto-close and fix false-positive detection May 9, 2026
@Eeshu-Yadav Eeshu-Yadav force-pushed the fix/stale-pr-bot-bot-reviewer-false-positives branch from bbc01e6 to 44b2d2a Compare May 9, 2026 22:14
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 2ce0ae8 and 44b2d2a.

📒 Files selected for processing (3)
  • .github/actions/bot-autoassign/stale_pr_bot.py
  • .github/actions/bot-autoassign/tests/test_stale_pr_bot.py
  • docs/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 TestGetLastChangesRequested exercises each path well.

Comment thread .github/actions/bot-autoassign/stale_pr_bot.py
coderabbitai[bot]
coderabbitai Bot previously approved these changes May 11, 2026
@nemesifier
Copy link
Copy Markdown
Member

nemesifier commented May 11, 2026

The latest commit (44b2d2a) addresses most of the prior review feedback. The following items remain.

Doc claim about push-vs-comment is now wrong

docs/developer/reusable-github-utils.rst:98-100 says:

An author comment on the PR clears the label and reassigns the issues; a push alone only resets the inactivity timer.

The new _clear_stale_label path in process_stale_prs means a push also clears the label and reassigns issues on the next daily run. The doc reads as if push doesn't clear, which is no longer true. Rework to something like:

The label clears and issues are reassigned automatically once the bot observes new author activity (next daily run); an author comment also triggers the immediate PRActivityBot path.

Stale label can still get stuck if the blocking review is later approved/dismissed

stale_pr_bot.py:400-401:

if not last_changes_requested:
    continue

Scenario: PR marked stale at day 14. At day 20 the reviewer approves (or dismisses their own review). On the next daily run, get_last_changes_requested returns None, the loop short-circuits before reaching the label-unwind block, and the stale label persists with linked issues still unassigned.

Cheap fix: hoist the _clear_stale_label + _reassign_unassigned_linked_issues check above the if not last_changes_requested gate, or run it unconditionally for any PR carrying the stale label.

Partial-unassign produces a false comment

unassign_linked_issues_helper (utils.py:105-111) swallows per-issue exceptions and returns a partial list, so mark_pr_stale only sees a successful call even when some issues failed. The comment still reads "the linked issue(s) have been unassigned from you" while one may still be assigned. The prior review suggested rewording to "we are unassigning...", the wording wasn't changed. Low-impact, but the fix is one string edit.

Two-line print concatenations still present

Most occurrences were cleaned up, but a few survive at stale_pr_bot.py:327-330, 413-416, 429-432. Trivial.

Behavioral change worth flagging (not a bug)

is_waiting_for_maintainer no longer treats maintainer comments as maintainer activity, only reviews. The docstring documents this ("Comments don't count") and test_maintainer_comment_does_not_count_as_review pins it. PRs where a maintainer asked for changes only through review comments or issue comments will no longer be considered "in maintainer court", they'll proceed to stale-warning. Worth confirming this matches team norms before merging.

PR title vs. actual scope

The PR title still says "Stop stale-PR bot from closing PRs blocked only by bot reviews," but the change also removes auto-close entirely and revamps the review-stance logic. Please always keep the PR title/description up to date.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 44b2d2a and 3091e79.

📒 Files selected for processing (3)
  • .github/actions/bot-autoassign/stale_pr_bot.py
  • .github/actions/bot-autoassign/tests/test_stale_pr_bot.py
  • docs/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

Comment thread .github/actions/bot-autoassign/stale_pr_bot.py Outdated
Comment thread .github/actions/bot-autoassign/stale_pr_bot.py Outdated
Comment thread .github/actions/bot-autoassign/tests/test_stale_pr_bot.py
@Eeshu-Yadav Eeshu-Yadav force-pushed the fix/stale-pr-bot-bot-reviewer-false-positives branch from 94003e2 to 9a3f8d0 Compare May 13, 2026 08:30
@nemesifier nemesifier changed the title [change] Stale-PR bot: drop auto-close and fix false-positive detection [ci:change] Stale-PR bot: drop auto-close and fix false-positive detection May 13, 2026
@nemesifier nemesifier merged commit 59e457d into openwisp:master May 13, 2026
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug github_actions Pull requests that update GitHub Actions code helper-bots Helper bots, release management automation

Development

Successfully merging this pull request may close these issues.

5 participants