[feature/bots] Automate assigning/unassigning issues #571 #572
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:
WalkthroughThis pull request introduces an automated GitHub issue assignment and pull request management system. The changes include four new GitHub Actions workflows that trigger on specific events (issue comments, PR opens/reopens, and a daily schedule) to execute Python-based bots. The bot system comprises three main implementations: one for handling assignment requests and PR-linked issue assignment, another for managing PR reopening and contributor activity, and a third for identifying and managing stale PRs based on inactivity thresholds. Configuration changes add PyGithub as an optional dependency, register the bot module in Django settings, and include comprehensive test coverage for all bot functionality. Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Event
participant WF as GitHub Action<br/>(Workflow)
participant Bot as Python Bot<br/>(Dispatcher)
participant Impl as Bot Implementation<br/>(Issue/PR/Stale)
participant API as GitHub API
GH->>WF: Trigger (issue_comment/<br/>pull_request/<br/>schedule)
WF->>WF: Checkout & Setup Python
WF->>WF: Install dependencies
WF->>Bot: Run with bot_type & event.json
Bot->>Impl: Dispatch to appropriate bot
Impl->>API: Load event payload
Impl->>API: Query repository & issues
Impl->>API: Perform action<br/>(assign/unassign/<br/>label/comment)
API-->>Impl: Response
Impl-->>Bot: Execution result
Bot-->>WF: Exit code
WF-->>GH: Complete
sequenceDiagram
participant Contributor as Contributor
participant GH as GitHub
participant Bot as IssueAssignmentBot
participant Repo as Repository
Contributor->>GH: Comment: "please assign"
GH->>Bot: Trigger issue_comment event
Bot->>Bot: Detect assignment request
Bot->>Repo: Fetch contributing guidelines
Repo-->>Bot: Guidelines URL
Bot->>GH: Post response with guidelines
Contributor->>GH: Open PR linking issue
GH->>Bot: Trigger pull_request event
Bot->>Repo: Extract linked issues
Bot->>Bot: Validate PR author
Bot->>GH: Auto-assign issue to PR author
GH-->>Contributor: Issue assigned
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces GitHub Actions workflows to automate issue assignment/unassignment based on PR linking and contributor activity, and to manage stale PRs after changes are requested.
Changes:
- Adds an “assign me” responder that directs contributors to the guidelines and encourages opening a linked PR.
- Auto-assigns linked issues to PR authors when PRs are opened/reopened.
- Introduces stale PR management that warns, marks stale/unassigns, and closes PRs after defined inactivity windows.
- Reassigns linked issues and removes stale status when PRs are reopened or when the PR author comments on a stale PR.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| .github/workflows/issue-assignment.yml | Responds to “assign me” issue comments with contribution guidance and PR-linking instructions. |
| .github/workflows/pr-issue-link.yml | Auto-assigns linked issues to PR authors on PR open/reopen. |
| .github/workflows/stale-pr-management.yml | Daily job to warn, mark stale/unassign, and close PRs after inactivity post “changes requested”. |
| .github/workflows/pr-reopen-reassign.yml | Reassigns issues / removes stale label on PR reopen and on PR-author follow-up comments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const prBody = pr.body || ''; | ||
| const issuePattern = /(?:fix(?:es)?|close[sd]?|resolve[sd]?)\s+#(\d+)/gi; | ||
| const matches = [...prBody.matchAll(issuePattern)]; | ||
| for (const match of matches) { | ||
| const issueNumber = parseInt(match[1], 10); | ||
| try { | ||
| await github.rest.issues.removeAssignees({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: issueNumber, | ||
| assignees: [prAuthor] | ||
| }); | ||
| console.log(`Unassigned ${prAuthor} from issue #${issueNumber}`); |
There was a problem hiding this comment.
When extracting linked issues from the PR body, duplicates aren’t deduplicated and PR references aren’t filtered out. This can lead to repeated API calls (and possible rate limiting) and may attempt to unassign from referenced pull requests. Consider tracking processedIssues (as done in other workflows) and fetching the item to skip issue.data.pull_request before unassigning.
|
|
||
| permissions: | ||
| issues: write | ||
| pull-requests: write |
There was a problem hiding this comment.
The workflow is granting pull-requests: write, but it only reads PR data (and modifies issues/labels). Reducing this to pull-requests: read limits token scope and avoids unnecessary write privileges.
| pull-requests: write | |
| pull-requests: read |
| name: 'stale' | ||
| }); | ||
| } catch (e) { | ||
|
|
There was a problem hiding this comment.
This empty catch block swallows failures when removing the stale label, which makes debugging harder (e.g., permission changes, API errors). Consider at least logging the error or explicitly handling the expected 404 case.
| const status = e?.status ?? e?.response?.status; | |
| if (status === 404) { | |
| console.log(`'stale' label not found on PR #${prNumber}, nothing to remove.`); | |
| } else { | |
| console.error(`Failed to remove 'stale' label from PR #${prNumber}:`, e); | |
| } |
.github/workflows/pr-issue-link.yml
Outdated
| const issuePattern = /(?:fix(?:es)?|close[sd]?|resolve[sd]?)\s+#(\d+)/gi; | ||
| const matches = [...prBody.matchAll(issuePattern)]; | ||
| if (matches.length === 0) { | ||
| console.log('No linked issues found in PR body'); | ||
| return; | ||
| } | ||
| const processedIssues = new Set(); | ||
| for (const match of matches) { | ||
| const issueNumber = parseInt(match[1], 10); | ||
| if (processedIssues.has(issueNumber)) { | ||
| continue; | ||
| } | ||
| processedIssues.add(issueNumber); | ||
| try { |
There was a problem hiding this comment.
The linked-issue parsing loop has no upper bound; a PR body referencing many issues could cause a large number of API calls and spam comments/assignments, potentially hitting rate limits. Consider limiting how many issue references are processed (e.g., first N unique matches) and logging when the limit is exceeded.
| with: | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| script: | | ||
| const contributingUrl = 'http://openwisp.io/docs/developer/contributing.html'; |
There was a problem hiding this comment.
The contributing URL is using plain HTTP. Prefer HTTPS to avoid mixed-content/security issues and ensure users are sent to the secure version of the docs.
| const contributingUrl = 'http://openwisp.io/docs/developer/contributing.html'; | |
| const contributingUrl = 'https://openwisp.io/docs/developer/contributing.html'; |
| contains(github.event.comment.body, 'assign this issue to me') || | ||
| contains(github.event.comment.body, 'assign me') || | ||
| contains(github.event.comment.body, 'can I work on this') || | ||
| contains(github.event.comment.body, 'I would like to work on this') || | ||
| contains(github.event.comment.body, 'I want to work on this') |
There was a problem hiding this comment.
The trigger conditions use case-sensitive contains(...) checks, so common variants like "Assign me" / "assign me!" / different capitalization won’t match. Consider normalizing github.event.comment.body (e.g., toLowerCase()) and matching more robustly to avoid missed requests.
| contains(github.event.comment.body, 'assign this issue to me') || | |
| contains(github.event.comment.body, 'assign me') || | |
| contains(github.event.comment.body, 'can I work on this') || | |
| contains(github.event.comment.body, 'I would like to work on this') || | |
| contains(github.event.comment.body, 'I want to work on this') | |
| contains(toLower(github.event.comment.body), 'assign this issue to me') || | |
| contains(toLower(github.event.comment.body), 'assign me') || | |
| contains(toLower(github.event.comment.body), 'can i work on this') || | |
| contains(toLower(github.event.comment.body), 'i would like to work on this') || | |
| contains(toLower(github.event.comment.body), 'i want to work on this') |
| const { data: pullRequests } = await github.rest.pulls.list({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| state: 'open', | ||
| per_page: 100 | ||
| }); |
There was a problem hiding this comment.
The workflow only fetches the first page of open PRs (per_page: 100) and does not paginate, so repos with >100 open PRs will silently skip the rest. Consider using github.paginate(...) (or manual page looping) to process all open PRs reliably.
| const { data: pullRequests } = await github.rest.pulls.list({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| state: 'open', | |
| per_page: 100 | |
| }); | |
| const pullRequests = await github.paginate( | |
| github.rest.pulls.list, | |
| { | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| state: 'open', | |
| per_page: 100 | |
| } | |
| ); |
| const { data: reviews } = await github.rest.pulls.listReviews({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: prNumber | ||
| }); | ||
| const changesRequestedReviews = reviews | ||
| .filter(r => r.state === 'CHANGES_REQUESTED') | ||
| .sort((a, b) => new Date(b.submitted_at) - new Date(a.submitted_at)); | ||
| if (changesRequestedReviews.length === 0) { | ||
| continue; | ||
| } | ||
| const lastChangesRequested = new Date(changesRequestedReviews[0].submitted_at); | ||
| const { data: commits } = await github.rest.pulls.listCommits({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: prNumber, | ||
| per_page: 100 | ||
| }); | ||
| const { data: comments } = await github.rest.issues.listComments({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: prNumber, | ||
| per_page: 100 | ||
| }); |
There was a problem hiding this comment.
listReviews, listCommits and issues.listComments are called without pagination (and listReviews doesn’t even set per_page). For long-running PRs with many commits/comments/reviews, activity and the latest CHANGES_REQUESTED timestamp can be missed, which can incorrectly mark/close PRs. Use github.paginate(...) for these endpoints (and/or increase per_page + loop).
| issue_comment: | ||
| types: [created] | ||
| push: | ||
|
|
There was a problem hiding this comment.
on: push is configured, but both jobs are gated to pull_request_target or issue_comment, so push events will trigger workflow runs that immediately skip all jobs. Removing the push trigger avoids noisy/empty runs and reduces Actions usage.
| @@ -0,0 +1,77 @@ | |||
| name: PR Issue Auto-Assignment | |||
There was a problem hiding this comment.
PR description ends with “Fixes #541”, but #541 appears to be an unrelated (already-merged) UI PR in this repository’s history. If the intent is to close an issue for this automation feature, the reference likely needs to point to the correct issue number (or be removed to avoid auto-closing the wrong item).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/issue-assignment.yml:
- Line 28: The contributingUrl constant is using an insecure HTTP URL; update
the value of contributingUrl to use the HTTPS scheme (i.e., replace
'http://openwisp.io/docs/developer/contributing.html' with
'https://openwisp.io/docs/developer/contributing.html') so the link is served
over TLS and avoids mixed-content/MITM risks.
In @.github/workflows/stale-pr-management.yml:
- Around line 26-59: The list calls (pulls.list, pulls.listReviews,
pulls.listCommits, issues.listComments) currently use per_page: 100 and will
miss results beyond the first page; replace each call with github.paginate(...)
using the corresponding endpoint.merge({...}) parameters (keep owner, repo,
pull_number/issue_number and state where applicable) to populate pullRequests,
reviews, commits, and comments with the full arrays; remove reliance on
single-page responses and ensure downstream code (e.g., filtering
changesRequestedReviews and accessing commits/comments) works with the full
arrays returned by paginate.
🧹 Nitpick comments (1)
.github/workflows/pr-reopen-reassign.yml (1)
3-9: Drop the unusedpushtrigger to avoid empty workflow runs.No job is enabled for
push, so these runs will always be skipped.🧹 Minimal cleanup
on: pull_request_target: types: [reopened] issue_comment: types: [created] - push:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/issue-assignment.yml.github/workflows/pr-issue-link.yml.github/workflows/pr-reopen-reassign.yml.github/workflows/stale-pr-management.yml
⏰ 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~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.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~=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.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Agent
🔇 Additional comments (1)
.github/workflows/pr-issue-link.yml (1)
20-75: Solid auto-assignment flow — looks safe and well-guarded.Nice dedupe + PR/assignee checks with clear logging.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Eeshu-Yadav
force-pushed
the
issues/571-automate-assigning-unassigning-issues
branch
2 times, most recently
from
a34efd6 to
a116660
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/issue-assignment.yml:
- Around line 13-57: The job 'if' condition uses unsupported toLower() calls
around github.event.comment.body; update the conditional to remove the toLower()
wrappers and call contains() directly on github.event.comment.body (e.g.,
replace contains(toLower(github.event.comment.body), 'assign me') with
contains(github.event.comment.body, 'assign me')) so the expressions use the
built-in case-insensitive contains() correctly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/issue-assignment.yml.github/workflows/pr-issue-link.yml.github/workflows/pr-reopen-reassign.yml.github/workflows/stale-pr-management.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pr-reopen-reassign.yml
🧰 Additional context used
🪛 actionlint (1.7.10)
.github/workflows/issue-assignment.yml
[error] 15-15: undefined function "toLower". available functions are "always", "cancelled", "contains", "endswith", "failure", "format", "fromjson", "hashfiles", "join", "startswith", "success", "tojson"
(expression)
[error] 16-16: undefined function "toLower". available functions are "always", "cancelled", "contains", "endswith", "failure", "format", "fromjson", "hashfiles", "join", "startswith", "success", "tojson"
(expression)
[error] 17-17: undefined function "toLower". available functions are "always", "cancelled", "contains", "endswith", "failure", "format", "fromjson", "hashfiles", "join", "startswith", "success", "tojson"
(expression)
[error] 18-18: undefined function "toLower". available functions are "always", "cancelled", "contains", "endswith", "failure", "format", "fromjson", "hashfiles", "join", "startswith", "success", "tojson"
(expression)
[error] 19-19: undefined function "toLower". available functions are "always", "cancelled", "contains", "endswith", "failure", "format", "fromjson", "hashfiles", "join", "startswith", "success", "tojson"
(expression)
⏰ 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.10 | django~=5.1.0
- GitHub Check: Python==3.12 | 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.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.0.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
🔇 Additional comments (7)
.github/workflows/pr-issue-link.yml (2)
3-10: Workflow trigger and permissions look good.Clear event scope and minimal permissions for the task.
20-85: Solid issue-link parsing and assignment flow.Dedupe + max cap reduce rate‑limit risk; skip logic for PRs/assigned issues is sensible.
.github/workflows/stale-pr-management.yml (5)
3-10: Schedule and permissions are appropriate.Daily cadence and write permissions align with automation responsibilities.
81-94: Bot-comment detection for stale actions is clear and consistent.The
botComments+ flags avoid duplicate warnings.
95-155: Closure flow is clear and user-friendly.Closes after the threshold and unassigns linked issues as expected.
156-219: Unassign + stale labeling branch looks solid.The messaging and unassign behavior align with the workflow intent.
220-244: Stale warning messaging is concise and actionable.Good guidance for contributors before escalation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@Eeshu-Yadav look at the work we're doing here:
#562
The same recommendations I've been giving there apply here, we should make something consistent:
- reusable actions that we can enable in all repos
- automated tests to give us confidence when maintaining/changing/iterating
- documentation for usage
- enable this in a test repo where we can test it and link to it in the PR description
- use Python
github-project-automation
bot
moved this to In progress
in OpenWISP Priorities for next releases
nemesifier
changed the title
okk |
|
@nemesifier https://github.com/issue-automation-org/issue-bot
stale one will be updated and after doing that , here updated code will be pushed based on the tested ymls and also python code for the bot will be updated |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Good to see that you're testing this, I'd like to see the testing done after the python port, otherwise we'll need to test the same things twice which is time consuming.
See my comment below.
Eeshu-Yadav
force-pushed
the
issues/571-automate-assigning-unassigning-issues
branch
from
a116660 to
30dfeb4
Compare
Eeshu-Yadav
force-pushed
the
issues/571-automate-assigning-unassigning-issues
branch
from
30dfeb4 to
3ccd173
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@openwisp_utils/bots/auto_assign/issue_assignment_bot_tests.py`:
- Line 110: The failing test assumes detect_issue_type returns "bug" but the
mock_issue used in issue_assignment_bot_tests.py has no bug/feature keywords, so
detect_issue_type returns None and the response won't contain the literal "Fixes
`#123`"; fix this by updating the test setup to give mock_issue a title or body
containing a bug keyword (e.g., include "bug") so detect_issue_type(...) yields
"bug" and the generated call_args will include "Fixes `#123`", or alternatively
change the assertion to match the generic linking output (e.g., assert both
"Fixes `#123`" and "Closes `#123`" are handled or assert the generic format) —
locate references to mock_issue and the detect_issue_type call in the test to
apply the change.
- Around line 99-110: The failing tests are caused by mock_issue.labels not
being iterable when detect_issue_type(issue) enumerates issue.labels; update the
test setup in test_respond_to_assignment_request_success (and similarly in
test_handle_issue_comment_assignment_request) to set mock_issue.labels to an
iterable (e.g., an empty list or list of Label-like mocks) so detect_issue_type
can iterate over it; locate the mock object named mock_issue in these tests and
assign mock_issue.labels = [] (or a list of mocks) before calling
IssueAssignmentBot.respond_to_assignment_request / the handler.
- Around line 6-12: Remove the unused imports PRReopenBot and PRActivityBot from
the import block so this test module only imports IssueAssignmentBot;
specifically, edit the try/except import section where IssueAssignmentBot,
PRReopenBot, and PRActivityBot are imported and delete PRReopenBot and
PRActivityBot (and their corresponding None assignments in the except block)
leaving only IssueAssignmentBot to avoid unused symbols in
issue_assignment_bot_tests.py.
In `@openwisp_utils/bots/auto_assign/issue_assignment_bot.py`:
- Around line 116-126: The current detect_issue_type function uses substring
checks against combined_text with bug_keywords and feature_keywords (variables
referenced) which causes false positives; update detect_issue_type to perform
whole-word, case-insensitive matching instead (e.g., compile each keyword into a
word-boundary regex like r"\bkeyword\b" or tokenize combined_text and compare
tokens) and use re.search or token membership for scoring so words like "tissue"
or "renew" do not match "issue"/"new"; preserve existing variables
(bug_keywords, feature_keywords, combined_text) and keep the scoring logic same
but based on whole-word matches.
In `@openwisp_utils/bots/auto_assign/pr_reopen_bot.py`:
- Around line 37-45: Duplicate extract_linked_issues logic exists in multiple
bots; extract to a shared module-level function and import it. Create a new
utility function (e.g., extract_linked_issues(pr_body)) in a shared utils module
(utils.py) with the regex logic and move the import re to the top of that
module; then replace PRReopenBot.extract_linked_issues,
PRActivityBot.extract_linked_issues, StalePRBot.extract_linked_issues, and
IssueAssignmentBot.extract_linked_issues to call the shared
utils.extract_linked_issues; ensure signatures remain identical and remove the
in-method import re occurrences.
- Around line 248-252: The current code always posts encouragement_message via
pr.create_issue_comment even when no stale label was removed or no issues were
reassigned; update the flow to check the actual outcomes of the operations
(e.g., the result/boolean returned by remove_stale_label/remove_label_from_pr
and the count/list returned by reassign_linked_issues or similar functions) and
only compose/post pr.create_issue_comment with the “stale status removed and
issues reassigned” text when stale_label_removed is true and
reassigned_issues_count > 0; otherwise craft and post a different, accurate
message (or skip posting) reflecting the real result. Ensure you reference the
existing encouragement_message variable and only call pr.create_issue_comment
once with the appropriate conditional message.
- Around line 285-290: The branch selecting PRActivityBot vs PRReopenBot uses
sys.argv[0] which is never the user mode, so PRActivityBot is unreachable;
change the selection to read an explicit mode argument (e.g., sys.argv[1]) or
use argparse/cli parsing in the module that contains the current logic (the
block that instantiates PRActivityBot and PRReopenBot), validate that an
argument like "activity" is provided, and default to PRReopenBot otherwise;
update the startup/entrypoint logic where main()/if __name__ == "__main__"
triggers the bot so it expects and documents the mode argument or register
separate entry points per bot.
In `@openwisp_utils/bots/auto_assign/pr_reopen_tests.py`:
- Around line 14-209: Remove the duplicated TestStalePRBot class from
pr_reopen_tests.py (the one defining TestStalePRBot, setUp/tearDown, and all
tests) so only the copy in stale_pr_tests.py remains; also remove any related
imports or fixtures in pr_reopen_tests.py that are only used by that class, and
delete the incorrect patch targeting
"openwisp_utils.bots.auto_assign.pr_reopen_bot.Github" (used in setUp) so tests
no longer attempt to mock the wrong symbol — this leaves StalePRBot tests only
in stale_pr_tests.py where the correct import and patch targeting
(stale_pr_bot.Github) is used.
- Around line 322-328: The test payload in test_handle_contributor_activity is
missing the "pull_request" key inside the "issue" object so
handle_contributor_activity short-circuits; update the event_payload in the test
to include an appropriate "pull_request" entry (e.g., an empty object or minimal
fields) inside issue so PRActivityBot.handle_contributor_activity sees a PR and
returns True as the test expects.
- Around line 350-358: The test function
test_handle_contributor_activity_not_author constructs an event_payload missing
the "pull_request" key so the PR-detection guard returns False; to exercise the
author-check logic, add a "pull_request" key (e.g.,
event_payload["issue"]["pull_request"] = {}) to the payload in that test (or
include it inline in the dict) so PRActivityBot.handle_contributor_activity
reaches the author check path that verifies the commenter is not the author.
In `@openwisp_utils/bots/auto_assign/stale_pr_bot.py`:
- Around line 93-107: The current has_bot_comment function uses fragile
substring matching (comment_type.lower() in comment.body.lower()) causing false
matches; change it to look for a dedicated HTML comment marker token instead
(e.g., "<!-- bot:stale_warning -->" or "<!-- bot:stale_mark -->") so checks are
unambiguous: update has_bot_comment to search for the exact marker string in
comment.body and then update the places that post comments (e.g., functions that
post the warning and mark comments such as warn_pr_stale and mark_pr_stale) to
include the corresponding marker in the posted body so the bot can reliably
detect which comment type exists.
- Around line 305-317: The substring check in has_bot_comment(pr, "stale")
incorrectly matches the earlier "stale warning" comment and blocks escalation to
mark_pr_stale; update the conditional logic so the checks are unambiguous —
e.g., change calls to has_bot_comment to look for exact comment types/keys
(distinct tokens like "stale" vs "stale warning" or a dedicated tag argument) or
add a stricter exact-match flag, then use those exact matches in the branches
around DAYS_BEFORE_UNASSIGN and DAYS_BEFORE_STALE_WARNING when deciding to call
mark_pr_stale or send_stale_warning and increment processed_count.
In `@openwisp_utils/bots/auto_assign/stale_pr_tests.py`:
- Around line 1-362: This file duplicates three test classes; keep only
TestStalePRBot here and remove the duplicated TestPRReopenBot and
TestPRActivityBot definitions from this file; then ensure the other test file
(pr_reopen_tests.py) contains TestPRReopenBot and TestPRActivityBot (if missing,
move their code there) so each test class exists in a single place, update
imports/patch targets if needed to reflect the single-location change, and run
tests to confirm no duplicated runs remain.
🧹 Nitpick comments (6)
setup.py (1)
66-68: Consider adding an upper-bound constraint on PyGithub.Other extras in this file use
~=or explicit upper bounds to prevent unexpected major-version breakage. An open-ended>=2.0.0could pull in a future PyGithub 3.x with breaking API changes.Suggested fix
"bots": [ - "PyGithub>=2.0.0", + "PyGithub>=2.0.0,<3.0.0", ],openwisp_utils/bots/auto_assign/pr_reopen_bot.py (1)
8-31: Duplicated__init__boilerplate across all bot classes — consider a shared base class.
PRReopenBot.__init__,PRActivityBot.__init__,StalePRBot.__init__, andIssueAssignmentBot.__init__all contain nearly identical GitHub client initialization logic. ABaseBotMixinorBaseBotclass would eliminate ~25 lines of duplication per class and centralize configuration.Also applies to: 159-176
openwisp_utils/bots/auto_assign/stale_pr_bot.py (2)
44-54: Performance: fetches all branch commits, not only those after changes were requested.
self.repo.get_commits(sha=pr.head.sha)returns every commit reachable from the head, then materializes the full list. For long-lived branches this can be hundreds or thousands of commits and API pages. Consider using thesinceparameter to limit to commits afterlast_changes_requested:Proposed fix
- commits = list(self.repo.get_commits(sha=pr.head.sha)) + commits = list( + self.repo.get_commits(sha=pr.head.sha, since=last_changes_requested) + )
277-284: Loading all open PRs into a list upfront can be expensive.
list(self.repo.get_pulls(state="open"))eagerly fetches every page. PyGithub'sget_pullsreturns aPaginatedListthat supports lazy iteration. Iterating directly avoids holding all PR objects in memory.Proposed fix
- open_prs = list(self.repo.get_pulls(state="open")) - print(f"Found {len(open_prs)} open pull requests") + open_prs = self.repo.get_pulls(state="open") + print(f"Processing open pull requests (count: {open_prs.totalCount})")openwisp_utils/bots/auto_assign/__main__.py (1)
4-36: LGTM on the dispatcher structure.The bot-type dispatch with lazy imports and
sys.argvnormalization is clean. Minor nit: Ruff suggests[sys.argv[0], *sys.argv[2:]]over concatenation on lines 17 and 26.Nit: more idiomatic list construction
- sys.argv = [sys.argv[0]] + sys.argv[2:] + sys.argv = [sys.argv[0], *sys.argv[2:]]openwisp_utils/bots/auto_assign/issue_assignment_bot_tests.py (1)
12-13: Missing blank line before class definition.PEP 8 requires two blank lines before a top-level class definition. There's no blank line between the
exceptblock and theclassstatement.PRActivityBot = None + + class TestIssueAssignmentBot(TestCase):
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/issue-assignment.yml.github/workflows/pr-issue-link.yml.github/workflows/pr-reopen-reassign.yml.github/workflows/stale-pr-management.ymlopenwisp_utils/bots/__init__.pyopenwisp_utils/bots/auto_assign/__init__.pyopenwisp_utils/bots/auto_assign/__main__.pyopenwisp_utils/bots/auto_assign/issue_assignment_bot.pyopenwisp_utils/bots/auto_assign/issue_assignment_bot_tests.pyopenwisp_utils/bots/auto_assign/pr_reopen_bot.pyopenwisp_utils/bots/auto_assign/pr_reopen_tests.pyopenwisp_utils/bots/auto_assign/stale_pr_bot.pyopenwisp_utils/bots/auto_assign/stale_pr_tests.pysetup.py
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/issue-assignment.yml
- .github/workflows/pr-reopen-reassign.yml
- .github/workflows/stale-pr-management.yml
- .github/workflows/pr-issue-link.yml
🧰 Additional context used
🧬 Code graph analysis (5)
openwisp_utils/bots/auto_assign/issue_assignment_bot_tests.py (1)
openwisp_utils/bots/auto_assign/issue_assignment_bot.py (10)
IssueAssignmentBot(8-403)is_assignment_request(37-52)extract_linked_issues(217-223)respond_to_assignment_request(128-215)auto_assign_issues_from_pr(225-282)unassign_issues_from_pr(284-317)load_event_payload(33-35)handle_issue_comment(319-349)handle_pull_request(351-380)run(382-403)
openwisp_utils/bots/auto_assign/__main__.py (3)
openwisp_utils/bots/auto_assign/issue_assignment_bot.py (1)
main(404-418)openwisp_utils/bots/auto_assign/pr_reopen_bot.py (1)
main(281-300)openwisp_utils/bots/auto_assign/stale_pr_bot.py (1)
main(346-349)
openwisp_utils/bots/auto_assign/stale_pr_tests.py (3)
openwisp_utils/bots/auto_assign/pr_reopen_bot.py (4)
extract_linked_issues(37-45)extract_linked_issues(182-190)load_event_payload(33-35)load_event_payload(178-180)openwisp_utils/bots/auto_assign/stale_pr_bot.py (2)
extract_linked_issues(109-115)get_last_changes_requested(75-91)openwisp_utils/bots/auto_assign/issue_assignment_bot.py (2)
extract_linked_issues(217-223)load_event_payload(33-35)
openwisp_utils/bots/auto_assign/pr_reopen_bot.py (3)
openwisp_utils/bots/auto_assign/issue_assignment_bot.py (4)
load_event_payload(33-35)extract_linked_issues(217-223)run(382-403)main(404-418)openwisp_utils/bots/auto_assign/stale_pr_bot.py (3)
extract_linked_issues(109-115)run(330-345)main(346-349)openwisp_utils/bots/auto_assign/__main__.py (1)
main(4-36)
openwisp_utils/bots/auto_assign/stale_pr_bot.py (3)
openwisp_utils/bots/auto_assign/issue_assignment_bot.py (3)
extract_linked_issues(217-223)run(382-403)main(404-418)openwisp_utils/bots/auto_assign/pr_reopen_bot.py (5)
extract_linked_issues(37-45)extract_linked_issues(182-190)run(139-158)run(261-280)main(281-300)openwisp_utils/bots/auto_assign/__main__.py (1)
main(4-36)
🪛 Ruff (0.14.14)
openwisp_utils/bots/auto_assign/__main__.py
[warning] 17-17: Consider [sys.argv[0], *sys.argv[2:]] instead of concatenation
Replace with [sys.argv[0], *sys.argv[2:]]
(RUF005)
[warning] 26-26: Consider [sys.argv[0], *sys.argv[2:]] instead of concatenation
Replace with [sys.argv[0], *sys.argv[2:]]
(RUF005)
[warning] 33-33: Consider moving this statement to an else block
(TRY300)
[warning] 34-34: Do not catch blind exception: Exception
(BLE001)
openwisp_utils/bots/auto_assign/issue_assignment_bot.py
[warning] 19-19: Do not catch blind exception: Exception
(BLE001)
[warning] 212-212: Consider moving this statement to an else block
(TRY300)
[warning] 213-213: Do not catch blind exception: Exception
(BLE001)
[warning] 274-274: Do not catch blind exception: Exception
(BLE001)
[warning] 279-279: Consider moving this statement to an else block
(TRY300)
[warning] 280-280: Do not catch blind exception: Exception
(BLE001)
[warning] 310-310: Do not catch blind exception: Exception
(BLE001)
[warning] 313-313: Consider moving this statement to an else block
(TRY300)
[warning] 315-315: Do not catch blind exception: Exception
(BLE001)
[warning] 345-345: Consider moving this statement to an else block
(TRY300)
[warning] 347-347: Do not catch blind exception: Exception
(BLE001)
[warning] 376-376: Consider moving this statement to an else block
(TRY300)
[warning] 378-378: Do not catch blind exception: Exception
(BLE001)
[warning] 399-399: Do not catch blind exception: Exception
(BLE001)
[warning] 415-415: Do not catch blind exception: Exception
(BLE001)
openwisp_utils/bots/auto_assign/pr_reopen_bot.py
[warning] 19-19: Do not catch blind exception: Exception
(BLE001)
[warning] 82-82: Do not catch blind exception: Exception
(BLE001)
[warning] 88-88: Consider moving this statement to an else block
(TRY300)
[warning] 90-90: Do not catch blind exception: Exception
(BLE001)
[warning] 108-108: Do not catch blind exception: Exception
(BLE001)
[warning] 135-135: Do not catch blind exception: Exception
(BLE001)
[warning] 154-154: Do not catch blind exception: Exception
(BLE001)
[warning] 170-170: Do not catch blind exception: Exception
(BLE001)
[warning] 225-225: Do not catch blind exception: Exception
(BLE001)
[warning] 245-245: Do not catch blind exception: Exception
(BLE001)
[warning] 255-255: Consider moving this statement to an else block
(TRY300)
[warning] 257-257: Do not catch blind exception: Exception
(BLE001)
[warning] 276-276: Do not catch blind exception: Exception
(BLE001)
[warning] 295-295: Do not catch blind exception: Exception
(BLE001)
openwisp_utils/bots/auto_assign/stale_pr_bot.py
[warning] 22-22: Do not catch blind exception: Exception
(BLE001)
[warning] 71-71: Do not catch blind exception: Exception
(BLE001)
[warning] 89-89: Do not catch blind exception: Exception
(BLE001)
[warning] 104-104: Consider moving this statement to an else block
(TRY300)
[warning] 105-105: Do not catch blind exception: Exception
(BLE001)
[warning] 136-136: Do not catch blind exception: Exception
(BLE001)
[warning] 139-139: Consider moving this statement to an else block
(TRY300)
[warning] 141-141: Do not catch blind exception: Exception
(BLE001)
[warning] 182-182: Consider moving this statement to an else block
(TRY300)
[warning] 184-184: Do not catch blind exception: Exception
(BLE001)
[warning] 225-225: Do not catch blind exception: Exception
(BLE001)
[warning] 232-232: Consider moving this statement to an else block
(TRY300)
[warning] 234-234: Do not catch blind exception: Exception
(BLE001)
[warning] 271-271: Consider moving this statement to an else block
(TRY300)
[warning] 273-273: Do not catch blind exception: Exception
(BLE001)
[warning] 321-321: Do not catch blind exception: Exception
(BLE001)
[warning] 327-327: Do not catch blind exception: Exception
(BLE001)
[warning] 340-340: Consider moving this statement to an else block
(TRY300)
[warning] 341-341: Do not catch blind exception: Exception
(BLE001)
⏰ 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.10 | django~=5.1.0
- GitHub Check: Python==3.12 | 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~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
🔇 Additional comments (9)
openwisp_utils/bots/auto_assign/stale_pr_bot.py (1)
145-186: LGTM on the stale lifecycle actions (close, mark stale, warn).The three-tier approach (warn at 7d, mark stale + unassign at 14d, close at 60d) aligns with the PR objectives. The messages are well-crafted and user-friendly. The
unassign_linked_issuesreuse is clean.Also applies to: 188-236, 238-275
openwisp_utils/bots/auto_assign/issue_assignment_bot.py (2)
37-52: LGTM on assignment-request detection.The phrase-based matching is appropriate for this use case — the phrases are specific enough to avoid false positives while covering common variations. Good approach.
225-282: LGTM onauto_assign_issues_from_pr.The
max_issuesguard, PR-vs-issue check, already-assigned check, and confirmation comment are all well-handled. Good defensive coding.openwisp_utils/bots/auto_assign/pr_reopen_bot.py (1)
148-148: The review comment is incorrect. According to official GitHub Actions documentation,GITHUB_EVENT_NAMEis"pull_request_target"when a workflow is triggered by thepull_request_targetevent. The code comparisons at line 148 (pr_reopen_bot.py) and line 393 (issue_assignment_bot.py) are correct. Both bots properly retrieveGITHUB_EVENT_NAMEviaos.environ.get("GITHUB_EVENT_NAME")and correctly check against"pull_request_target". No changes are needed.openwisp_utils/bots/auto_assign/issue_assignment_bot_tests.py (5)
165-179: LGTM!The rate-limiting test correctly validates that only
max_issuesare processed, and the assertion on count is order-independent.
316-338: LGTM!Proper end-to-end test for the
pull_request_targetevent path with correctly configured mocks.
38-49: LGTM!Initialization tests correctly verify both the happy path and missing-env-var scenarios.
51-79: LGTM!Good coverage of both positive and negative cases for assignment request detection, including edge cases like empty strings and
None.
81-97: LGTM!Solid test for linked issue extraction with deduplication and edge cases.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Eeshu-Yadav
force-pushed
the
issues/571-automate-assigning-unassigning-issues
branch
4 times, most recently
from
92c7e63 to
374ace8
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@openwisp_utils/bots/auto_assign/issue_assignment_bot_tests.py`:
- Around line 307-323: The test test_run_issue_comment_event creates mock_issue
= Mock() but doesn't set iterable labels or strings for title/body, causing
detect_issue_type to iterate over a Mock and raise; fix by configuring
mock_issue in that test: set mock_issue.labels to an iterable (e.g., an empty
list), and set mock_issue.title and mock_issue.body to simple string values so
detect_issue_type and respond_to_assignment_request can operate normally; locate
the mock_issue assignment in test_run_issue_comment_event and update those
attributes accordingly.
- Around line 109-113: The test's expectation/comment is wrong because
mock_issue.title contains the bug keyword "issue", so detect_issue_type returns
"bug" and the code emits the bug-specific backreference format; either update
the test data to avoid bug/feature keywords (change mock_issue.title from "Test
issue title" to a neutral title like "Test task title" or remove "issue" from
body) so the generic branch is exercised, or else update the assertion/comment
to reflect the bug-specific path (assert that the comment contains the exact
bug-format string such as "Fixes `#123`" and update the inline comment on the
assertion); locate this in the test where mock_issue.create_comment is inspected
(call_args variable) and where mock_issue.title is set.
In `@openwisp_utils/bots/auto_assign/issue_assignment_bot.py`:
- Around line 60-135: The detect_issue_type function currently biases results
because the token "issue" in bug_keywords will match almost every GitHub issue;
update detect_issue_type by removing "issue" from the bug_keywords list (or
alternatively treating it as a low-weight token) and adjust the scoring logic
that computes bug_score/feature_score so common generic tokens don't dominate
(refer to bug_keywords, feature_keywords, detect_issue_type, bug_score and
feature_score); implement the change by either deleting "issue" from
bug_keywords or subtracting a small weight when matching that specific keyword
before comparing bug_score and feature_score.
In `@openwisp_utils/bots/auto_assign/pr_reopen_bot.py`:
- Around line 114-139: handle_pr_reopen currently returns True only if
reassign_issues_to_author returned >0, so removing a stale label without
reassigning still yields False; update handle_pr_reopen to capture the result of
remove_stale_label (e.g., removed = self.remove_stale_label(pr_number)) and
return True if either reassigned has length > 0 OR removed is truthy; if
remove_stale_label doesn't return a boolean yet, adjust it to return True on
successful removal (or at least a truthy value) so you can combine: return
(len(reassigned) > 0) or bool(removed). Ensure references to handle_pr_reopen,
reassign_issues_to_author, and remove_stale_label are used to locate the
changes.
In `@openwisp_utils/bots/auto_assign/stale_pr_bot.py`:
- Around line 43-71: The code currently calls
self.repo.get_commits(sha=pr.head.sha) which returns the entire reachable
history and can be huge; replace that call with pr.get_commits() to iterate only
the commits belonging to the pull request (keep the existing commit/date logic
and last_author_activity variable), and avoid materializing all commits at once
(iterate the PR commits generator instead of wrapping with list()) to reduce API
pagination and rate-limit usage in the methods like the stale PR age computation
used by process_stale_prs.
- Around line 317-322: The dedupe check in process_stale_prs is using
has_bot_comment(pr, "stale warning") and has_bot_comment(pr, "reminder") which
don't match the actual marker posted by send_stale_warning (<!--
bot:stale_warning -->) and no code ever posts <!-- bot:reminder -->; update
process_stale_prs to call has_bot_comment(pr, "stale_warning") (underscore)
instead of "stale warning" and remove the dead has_bot_comment(pr, "reminder")
check (or replace it only if you add a matching reminder marker elsewhere),
ensuring deduplication uses the same marker string as send_stale_warning.
- Around line 310-315: The check for has_bot_comment(pr, "unassigned") is dead
code because no method ever posts the "<!-- bot:unassigned -->" marker; remove
that check from the conditional in stale_pr_bot.py (the block around
DAYS_BEFORE_UNASSIGN and mark_pr_stale) so the branch simply tests
has_bot_comment(pr, "stale") and then calls mark_pr_stale(pr, days_inactive) as
before, leaving DAYS_BEFORE_UNASSIGN, has_bot_comment, and mark_pr_stale intact.
In `@openwisp_utils/bots/auto_assign/stale_pr_tests.py`:
- Around line 139-152: The test test_has_bot_comment sets mock_comment.body to
"This is a stale warning" but has_bot_comment (StalePRBot.has_bot_comment) looks
for the HTML marker "<!-- bot:stale -->"; update the test so mock_comment.body
includes that exact marker (e.g., include "<!-- bot:stale --> This is a stale
warning") when calling bot.has_bot_comment(mock_pr, "stale"), and keep a body
without the marker for the "closed" assertion to ensure the True/False
expectations are correct.
🧹 Nitpick comments (4)
openwisp_utils/bots/auto_assign/pr_reopen_bot.py (1)
10-33: Duplicated__init__and GitHub client setup across bot classes — consider a shared base class.
PRReopenBot.__init__,PRActivityBot.__init__,StalePRBot.__init__, andIssueAssignmentBot.__init__all contain the same token/repo/client initialization logic. ABaseBotMixinor base class would eliminate ~25 lines of duplication per bot.Also applies to: 163-180
openwisp_utils/bots/auto_assign/__main__.py (1)
15-29: Mutatingsys.argvglobally is a side effect that persists after exceptions.If
issue_main()orpr_main()raises,sys.argvhas already been overwritten on lines 19/28. This is acceptable for a CLI entry point that exits after one run, but consider passing the payload path as a function argument instead for testability.openwisp_utils/bots/auto_assign/stale_pr_bot.py (1)
148-189:close_stale_prdoes not include a<!-- bot:closed -->HTML marker in the close comment.This isn't currently a problem since closed PRs are filtered out by
get_pulls(state="open"), but adding a marker would be consistent with the other bot comments and useful if the check is ever needed.openwisp_utils/bots/auto_assign/issue_assignment_bot.py (1)
415-433:IssueAssignmentBot()is instantiated (and GitHub client initialized) before argument validation.Line 420 creates the bot and authenticates with GitHub even when invoked with no arguments. Moving instantiation after the
sys.argvcheck avoids unnecessary API calls.Proposed fix
def main(): """Entry point for command line usage""" import json import sys - bot = IssueAssignmentBot() if len(sys.argv) > 1: + bot = IssueAssignmentBot() try: with open(sys.argv[1], "r") as f: event_payload = json.load(f) bot.load_event_payload(event_payload) except Exception as e: print(f"Could not load event payload: {e}") - - bot.run() + bot.run() + else: + print("Usage: python issue_assignment_bot.py <event_payload.json>")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/issue-assignment.yml.github/workflows/pr-issue-link.yml.github/workflows/pr-reopen-reassign.yml.github/workflows/stale-pr-management.ymlopenwisp_utils/bots/__init__.pyopenwisp_utils/bots/auto_assign/__init__.pyopenwisp_utils/bots/auto_assign/__main__.pyopenwisp_utils/bots/auto_assign/issue_assignment_bot.pyopenwisp_utils/bots/auto_assign/issue_assignment_bot_tests.pyopenwisp_utils/bots/auto_assign/pr_reopen_bot.pyopenwisp_utils/bots/auto_assign/pr_reopen_tests.pyopenwisp_utils/bots/auto_assign/stale_pr_bot.pyopenwisp_utils/bots/auto_assign/stale_pr_tests.pysetup.py
🚧 Files skipped from review as they are similar to previous changes (6)
- .github/workflows/issue-assignment.yml
- .github/workflows/pr-reopen-reassign.yml
- .github/workflows/stale-pr-management.yml
- openwisp_utils/bots/auto_assign/pr_reopen_tests.py
- .github/workflows/pr-issue-link.yml
- setup.py
🧰 Additional context used
🧬 Code graph analysis (5)
openwisp_utils/bots/auto_assign/stale_pr_tests.py (1)
openwisp_utils/bots/auto_assign/stale_pr_bot.py (9)
StalePRBot(12-350)extract_linked_issues(112-118)get_last_changes_requested(77-93)get_days_since_activity(38-75)unassign_linked_issues(120-146)has_bot_comment(95-110)send_stale_warning(242-280)mark_pr_stale(191-240)close_stale_pr(148-189)
openwisp_utils/bots/auto_assign/pr_reopen_bot.py (3)
openwisp_utils/bots/auto_assign/issue_assignment_bot.py (4)
load_event_payload(35-37)extract_linked_issues(226-232)run(391-412)main(415-429)openwisp_utils/bots/auto_assign/stale_pr_bot.py (3)
extract_linked_issues(112-118)run(335-350)main(353-356)openwisp_utils/bots/auto_assign/__main__.py (1)
main(6-38)
openwisp_utils/bots/auto_assign/issue_assignment_bot_tests.py (1)
openwisp_utils/bots/auto_assign/issue_assignment_bot.py (10)
IssueAssignmentBot(10-412)is_assignment_request(39-54)extract_linked_issues(226-232)respond_to_assignment_request(137-224)auto_assign_issues_from_pr(234-291)unassign_issues_from_pr(293-326)load_event_payload(35-37)handle_issue_comment(328-358)handle_pull_request(360-389)run(391-412)
openwisp_utils/bots/auto_assign/issue_assignment_bot.py (2)
openwisp_utils/bots/auto_assign/pr_reopen_bot.py (7)
load_event_payload(35-37)load_event_payload(182-184)extract_linked_issues(39-47)extract_linked_issues(186-194)run(141-160)run(265-284)main(287-309)openwisp_utils/bots/auto_assign/stale_pr_bot.py (3)
extract_linked_issues(112-118)run(335-350)main(353-356)
openwisp_utils/bots/auto_assign/__main__.py (3)
openwisp_utils/bots/auto_assign/issue_assignment_bot.py (1)
main(415-429)openwisp_utils/bots/auto_assign/pr_reopen_bot.py (1)
main(287-309)openwisp_utils/bots/auto_assign/stale_pr_bot.py (1)
main(353-356)
🪛 Ruff (0.14.14)
openwisp_utils/bots/auto_assign/pr_reopen_bot.py
[warning] 21-21: Do not catch blind exception: Exception
(BLE001)
[warning] 84-84: Do not catch blind exception: Exception
(BLE001)
[warning] 90-90: Consider moving this statement to an else block
(TRY300)
[warning] 92-92: Do not catch blind exception: Exception
(BLE001)
[warning] 110-110: Do not catch blind exception: Exception
(BLE001)
[warning] 137-137: Do not catch blind exception: Exception
(BLE001)
[warning] 156-156: Do not catch blind exception: Exception
(BLE001)
[warning] 174-174: Do not catch blind exception: Exception
(BLE001)
[warning] 229-229: Do not catch blind exception: Exception
(BLE001)
[warning] 249-249: Do not catch blind exception: Exception
(BLE001)
[warning] 259-259: Consider moving this statement to an else block
(TRY300)
[warning] 261-261: Do not catch blind exception: Exception
(BLE001)
[warning] 280-280: Do not catch blind exception: Exception
(BLE001)
[warning] 306-306: Do not catch blind exception: Exception
(BLE001)
openwisp_utils/bots/auto_assign/stale_pr_bot.py
[warning] 24-24: Do not catch blind exception: Exception
(BLE001)
[warning] 73-73: Do not catch blind exception: Exception
(BLE001)
[warning] 91-91: Do not catch blind exception: Exception
(BLE001)
[warning] 107-107: Consider moving this statement to an else block
(TRY300)
[warning] 108-108: Do not catch blind exception: Exception
(BLE001)
[warning] 139-139: Do not catch blind exception: Exception
(BLE001)
[warning] 142-142: Consider moving this statement to an else block
(TRY300)
[warning] 144-144: Do not catch blind exception: Exception
(BLE001)
[warning] 185-185: Consider moving this statement to an else block
(TRY300)
[warning] 187-187: Do not catch blind exception: Exception
(BLE001)
[warning] 229-229: Do not catch blind exception: Exception
(BLE001)
[warning] 236-236: Consider moving this statement to an else block
(TRY300)
[warning] 238-238: Do not catch blind exception: Exception
(BLE001)
[warning] 276-276: Consider moving this statement to an else block
(TRY300)
[warning] 278-278: Do not catch blind exception: Exception
(BLE001)
[warning] 326-326: Do not catch blind exception: Exception
(BLE001)
[warning] 332-332: Do not catch blind exception: Exception
(BLE001)
[warning] 345-345: Consider moving this statement to an else block
(TRY300)
[warning] 346-346: Do not catch blind exception: Exception
(BLE001)
openwisp_utils/bots/auto_assign/issue_assignment_bot.py
[warning] 21-21: Do not catch blind exception: Exception
(BLE001)
[warning] 221-221: Consider moving this statement to an else block
(TRY300)
[warning] 222-222: Do not catch blind exception: Exception
(BLE001)
[warning] 283-283: Do not catch blind exception: Exception
(BLE001)
[warning] 288-288: Consider moving this statement to an else block
(TRY300)
[warning] 289-289: Do not catch blind exception: Exception
(BLE001)
[warning] 319-319: Do not catch blind exception: Exception
(BLE001)
[warning] 322-322: Consider moving this statement to an else block
(TRY300)
[warning] 324-324: Do not catch blind exception: Exception
(BLE001)
[warning] 354-354: Consider moving this statement to an else block
(TRY300)
[warning] 356-356: Do not catch blind exception: Exception
(BLE001)
[warning] 385-385: Consider moving this statement to an else block
(TRY300)
[warning] 387-387: Do not catch blind exception: Exception
(BLE001)
[warning] 408-408: Do not catch blind exception: Exception
(BLE001)
[warning] 426-426: Do not catch blind exception: Exception
(BLE001)
openwisp_utils/bots/auto_assign/__main__.py
[warning] 19-19: Consider [sys.argv[0], *sys.argv[2:]] instead of concatenation
Replace with [sys.argv[0], *sys.argv[2:]]
(RUF005)
[warning] 28-28: Consider [sys.argv[0], *sys.argv[2:]] instead of concatenation
Replace with [sys.argv[0], *sys.argv[2:]]
(RUF005)
[warning] 35-35: Consider moving this statement to an else block
(TRY300)
[warning] 36-36: Do not catch blind exception: Exception
(BLE001)
⏰ 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.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.2.0
🔇 Additional comments (10)
openwisp_utils/bots/auto_assign/pr_reopen_bot.py (1)
287-309:main()continues tobot.run()even when payload loading fails.If the
json.loadon line 295 oropenon line 294 raises, the exception is caught and printed (line 307), butbot.run()is never reached either because theexceptends thetryblock. However, ifbot.load_event_payloaditself raised,bot.run()would still be skipped since it's inside the sametry. On closer inspection, the entire dispatch is inside thetry, so a payload error aborts execution. This is fine.openwisp_utils/bots/auto_assign/stale_pr_tests.py (3)
1-11: Test setup and structure look solid.The
setUp/tearDownproperly patch environment variables and theGithubclass. TheskipTestguard for missing imports is a good defensive pattern.
46-62: Good coverage ofextract_linked_issuesedge cases including duplicates, empty, and None.
96-118: The datetime mock is safe—no action needed.The code imports
datetimeandtimezoneseparately (from datetime import datetime, timezone), and uses onlydatetime.now(timezone.utc). Sincetimezoneis not accessed as an attribute of thedatetimeobject (nodatetime.timezonepatterns found), patching only thedatetimeclass poses no risk.openwisp_utils/bots/auto_assign/__main__.py (1)
1-42: Dispatcher logic is clear and functional.The bot-type routing, lazy imports, and error handling are well-structured for a CLI entry point.
openwisp_utils/bots/auto_assign/issue_assignment_bot_tests.py (3)
12-36: Test setup and teardown are well-structured.Clean environment patching with proper cleanup, and the
Githubclass is correctly mocked at the right import path.
50-96: Thorough coverage of assignment request detection and linked issues extraction.Good use of
subTestfor parameterized cases, including edge cases (empty, None, duplicates).
121-169: Auto-assign tests cover key scenarios well: success, already-assigned, PR references, and rate limiting.openwisp_utils/bots/auto_assign/stale_pr_bot.py (1)
95-110: HTML marker approach forhas_bot_commentis a solid improvement over substring matching.openwisp_utils/bots/auto_assign/issue_assignment_bot.py (1)
234-291:auto_assign_issues_from_pr— rate-limiting by truncating the list is a good safeguard.The
max_issuesparameter and early truncation on line 251 prevent excessive API calls when a PR body references many issues.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| call_args = mock_issue.create_comment.call_args[0][0] | ||
| self.assertIn("@testuser", call_args) | ||
| self.assertIn("contributing guidelines", call_args) | ||
| # Generic format includes both Fixes and Closes | ||
| self.assertTrue("Fixes `#123`" in call_args or "Closes `#123`" in call_args) |
There was a problem hiding this comment.
Misleading test comment: this actually hits the bug-specific path, not the generic format.
The mock has title="Test issue title", which contains the word "issue" — a bug keyword. So detect_issue_type returns "bug" and the message uses `Fixes `#123 ``, not the generic format. The comment on line 112 ("Generic format includes both Fixes and Closes") is inaccurate.
Fix the comment or the test data
Either update the comment:
- # Generic format includes both Fixes and Closes
- self.assertTrue("Fixes `#123`" in call_args or "Closes `#123`" in call_args)
+ # "issue" in title triggers bug detection → Fixes keyword
+ self.assertIn("Fixes `#123`", call_args)Or use a title/body without bug/feature keywords to test the generic path:
- mock_issue.title = "Test issue title"
- mock_issue.body = "Test issue body"
+ mock_issue.title = "Some task"
+ mock_issue.body = "Description of task"🤖 Prompt for AI Agents
In `@openwisp_utils/bots/auto_assign/issue_assignment_bot_tests.py` around lines
109 - 113, The test's expectation/comment is wrong because mock_issue.title
contains the bug keyword "issue", so detect_issue_type returns "bug" and the
code emits the bug-specific backreference format; either update the test data to
avoid bug/feature keywords (change mock_issue.title from "Test issue title" to a
neutral title like "Test task title" or remove "issue" from body) so the generic
branch is exercised, or else update the assertion/comment to reflect the
bug-specific path (assert that the comment contains the exact bug-format string
such as "Fixes `#123`" and update the inline comment on the assertion); locate
this in the test where mock_issue.create_comment is inspected (call_args
variable) and where mock_issue.title is set.
| def test_run_issue_comment_event(self): | ||
| bot = IssueAssignmentBot() | ||
| bot.event_name = "issue_comment" | ||
|
|
||
| event_payload = { | ||
| "issue": {"number": 123, "pull_request": None}, | ||
| "comment": {"body": "assign me", "user": {"login": "testuser"}}, | ||
| } | ||
|
|
||
| bot.load_event_payload(event_payload) | ||
|
|
||
| mock_issue = Mock() | ||
| self.mock_repo.get_issue.return_value = mock_issue | ||
|
|
||
| result = bot.run() | ||
|
|
||
| self.assertTrue(result) |
There was a problem hiding this comment.
Bug: test_run_issue_comment_event will fail — mock_issue.labels is not iterable.
mock_issue = Mock() on line 318 doesn't configure labels, title, or body. When detect_issue_type iterates over issue.labels (a bare Mock), it raises TypeError, which is caught by the outer except, causing respond_to_assignment_request to return False. The assertion on line 323 (assertTrue) will fail.
🐛 Proposed fix
def test_run_issue_comment_event(self):
bot = IssueAssignmentBot()
bot.event_name = "issue_comment"
event_payload = {
"issue": {"number": 123, "pull_request": None},
"comment": {"body": "assign me", "user": {"login": "testuser"}},
}
bot.load_event_payload(event_payload)
mock_issue = Mock()
+ mock_issue.labels = []
+ mock_issue.title = "Test issue"
+ mock_issue.body = "Test body"
self.mock_repo.get_issue.return_value = mock_issue
result = bot.run()
self.assertTrue(result)🤖 Prompt for AI Agents
In `@openwisp_utils/bots/auto_assign/issue_assignment_bot_tests.py` around lines
307 - 323, The test test_run_issue_comment_event creates mock_issue = Mock() but
doesn't set iterable labels or strings for title/body, causing detect_issue_type
to iterate over a Mock and raise; fix by configuring mock_issue in that test:
set mock_issue.labels to an iterable (e.g., an empty list), and set
mock_issue.title and mock_issue.body to simple string values so
detect_issue_type and respond_to_assignment_request can operate normally; locate
the mock_issue assignment in test_run_issue_comment_event and update those
attributes accordingly.
| def detect_issue_type(self, issue): | ||
| """Intelligently detect issue type. | ||
|
|
||
| Analyzes labels, title and body and returns 'bug', 'feature', or | ||
| None. | ||
| """ | ||
| bug_keywords = [ | ||
| "bug", | ||
| "error", | ||
| "crash", | ||
| "fail", | ||
| "broken", | ||
| "issue", | ||
| "problem", | ||
| "not working", | ||
| "doesn't work", | ||
| "does not work", | ||
| "fix", | ||
| "incorrect", | ||
| "wrong", | ||
| "exception", | ||
| "traceback", | ||
| "breaking", | ||
| "regression", | ||
| ] | ||
|
|
||
| feature_keywords = [ | ||
| "feature", | ||
| "enhancement", | ||
| "add", | ||
| "implement", | ||
| "support", | ||
| "new", | ||
| "create", | ||
| "allow", | ||
| "enable", | ||
| "improve", | ||
| "improvement", | ||
| "upgrade", | ||
| "extend", | ||
| "functionality", | ||
| "capability", | ||
| "ability", | ||
| "option", | ||
| ] | ||
|
|
||
| issue_labels = [label.name.lower() for label in issue.labels] | ||
| if any(label in issue_labels for label in ["bug", "bugfix", "fix"]): | ||
| return "bug" | ||
| elif any( | ||
| label in issue_labels for label in ["feature", "enhancement", "improvement"] | ||
| ): | ||
| return "feature" | ||
|
|
||
| title = (issue.title or "").lower() | ||
| body = (issue.body or "").lower() | ||
| combined_text = f"{title} {body}" | ||
|
|
||
| # Use word boundary matching to avoid false positives | ||
| bug_score = sum( | ||
| 1 | ||
| for keyword in bug_keywords | ||
| if re.search(rf"\b{re.escape(keyword)}\b", combined_text) | ||
| ) | ||
| feature_score = sum( | ||
| 1 | ||
| for keyword in feature_keywords | ||
| if re.search(rf"\b{re.escape(keyword)}\b", combined_text) | ||
| ) | ||
|
|
||
| if bug_score > feature_score and bug_score > 0: | ||
| return "bug" | ||
| elif feature_score > bug_score and feature_score > 0: | ||
| return "feature" | ||
|
|
||
| return None |
There was a problem hiding this comment.
"issue" in bug_keywords will cause most GitHub issues to be classified as bugs.
Nearly every issue description or title on a GitHub issue tracker will contain the word "issue" (e.g., "This issue tracks…", "The issue is…"). With word-boundary matching this won't false-positive on substrings, but the word itself is ubiquitous in this context. This biases detect_issue_type toward "bug" for most issues.
Consider removing "issue" from bug_keywords or down-weighting it.
🤖 Prompt for AI Agents
In `@openwisp_utils/bots/auto_assign/issue_assignment_bot.py` around lines 60 -
135, The detect_issue_type function currently biases results because the token
"issue" in bug_keywords will match almost every GitHub issue; update
detect_issue_type by removing "issue" from the bug_keywords list (or
alternatively treating it as a low-weight token) and adjust the scoring logic
that computes bug_score/feature_score so common generic tokens don't dominate
(refer to bug_keywords, feature_keywords, detect_issue_type, bug_score and
feature_score); implement the change by either deleting "issue" from
bug_keywords or subtracting a small weight when matching that specific keyword
before comparing bug_score and feature_score.
| def handle_pr_reopen(self): | ||
| """Handle PR reopening event""" | ||
| if not self.event_payload: | ||
| print("No event payload available") | ||
| return False | ||
|
|
||
| try: | ||
| pr = self.event_payload.get("pull_request", {}) | ||
| pr_number = pr.get("number") | ||
| pr_author = pr.get("user", {}).get("login", "") | ||
| pr_body = pr.get("body", "") | ||
|
|
||
| if not all([pr_number, pr_author]): | ||
| print("Missing required PR data") | ||
| return False | ||
|
|
||
| print(f"Handling PR #{pr_number} reopen by {pr_author}") | ||
| reassigned = self.reassign_issues_to_author(pr_number, pr_author, pr_body) | ||
| self.remove_stale_label(pr_number) | ||
|
|
||
| print(f"Reassigned {len(reassigned)} issues to {pr_author}") | ||
| return len(reassigned) > 0 | ||
|
|
||
| except Exception as e: | ||
| print(f"Error handling PR reopen: {e}") | ||
| return False |
There was a problem hiding this comment.
handle_pr_reopen returns False when stale label was removed but no issues were reassigned.
Line 135 only checks len(reassigned) > 0. If the PR had no linked issues but did have a stale label, the label is removed but the method reports failure. Consider combining both outcomes.
Proposed fix
- reassigned = self.reassign_issues_to_author(pr_number, pr_author, pr_body)
- self.remove_stale_label(pr_number)
+ reassigned = self.reassign_issues_to_author(pr_number, pr_author, pr_body)
+ label_removed = self.remove_stale_label(pr_number)
print(f"Reassigned {len(reassigned)} issues to {pr_author}")
- return len(reassigned) > 0
+ return len(reassigned) > 0 or label_removed🧰 Tools
🪛 Ruff (0.14.14)
[warning] 137-137: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@openwisp_utils/bots/auto_assign/pr_reopen_bot.py` around lines 114 - 139,
handle_pr_reopen currently returns True only if reassign_issues_to_author
returned >0, so removing a stale label without reassigning still yields False;
update handle_pr_reopen to capture the result of remove_stale_label (e.g.,
removed = self.remove_stale_label(pr_number)) and return True if either
reassigned has length > 0 OR removed is truthy; if remove_stale_label doesn't
return a boolean yet, adjust it to return True on successful removal (or at
least a truthy value) so you can combine: return (len(reassigned) > 0) or
bool(removed). Ensure references to handle_pr_reopen, reassign_issues_to_author,
and remove_stale_label are used to locate the changes.
| try: | ||
| pr_author = pr.user.login | ||
| last_author_activity = None | ||
|
|
||
| commits = list(self.repo.get_commits(sha=pr.head.sha)) | ||
| for commit in commits: | ||
| commit_date = commit.commit.author.date | ||
| if commit_date > last_changes_requested: | ||
| if commit.author and commit.author.login == pr_author: | ||
| if ( | ||
| not last_author_activity | ||
| or commit_date > last_author_activity | ||
| ): | ||
| last_author_activity = commit_date | ||
|
|
||
| comments = list(pr.get_issue_comments()) | ||
| for comment in comments: | ||
| if comment.user.login == pr_author: | ||
| comment_date = comment.created_at | ||
| if comment_date > last_changes_requested: | ||
| if ( | ||
| not last_author_activity | ||
| or comment_date > last_author_activity | ||
| ): | ||
| last_author_activity = comment_date | ||
|
|
||
| reference_date = last_author_activity or last_changes_requested | ||
| now = datetime.now(timezone.utc) | ||
| return (now - reference_date).days |
There was a problem hiding this comment.
self.repo.get_commits(sha=pr.head.sha) fetches all reachable commits, not just PR commits.
For a branch based on main, this returns the entire commit history reachable from that SHA. This can be hundreds or thousands of commits, causing significant API pagination overhead and potential rate-limit exhaustion, especially when processing multiple PRs in process_stale_prs.
Use pr.get_commits() instead to fetch only the PR's own commits.
Proposed fix
- commits = list(self.repo.get_commits(sha=pr.head.sha))
+ commits = list(pr.get_commits())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| pr_author = pr.user.login | |
| last_author_activity = None | |
| commits = list(self.repo.get_commits(sha=pr.head.sha)) | |
| for commit in commits: | |
| commit_date = commit.commit.author.date | |
| if commit_date > last_changes_requested: | |
| if commit.author and commit.author.login == pr_author: | |
| if ( | |
| not last_author_activity | |
| or commit_date > last_author_activity | |
| ): | |
| last_author_activity = commit_date | |
| comments = list(pr.get_issue_comments()) | |
| for comment in comments: | |
| if comment.user.login == pr_author: | |
| comment_date = comment.created_at | |
| if comment_date > last_changes_requested: | |
| if ( | |
| not last_author_activity | |
| or comment_date > last_author_activity | |
| ): | |
| last_author_activity = comment_date | |
| reference_date = last_author_activity or last_changes_requested | |
| now = datetime.now(timezone.utc) | |
| return (now - reference_date).days | |
| try: | |
| pr_author = pr.user.login | |
| last_author_activity = None | |
| commits = list(pr.get_commits()) | |
| for commit in commits: | |
| commit_date = commit.commit.author.date | |
| if commit_date > last_changes_requested: | |
| if commit.author and commit.author.login == pr_author: | |
| if ( | |
| not last_author_activity | |
| or commit_date > last_author_activity | |
| ): | |
| last_author_activity = commit_date | |
| comments = list(pr.get_issue_comments()) | |
| for comment in comments: | |
| if comment.user.login == pr_author: | |
| comment_date = comment.created_at | |
| if comment_date > last_changes_requested: | |
| if ( | |
| not last_author_activity | |
| or comment_date > last_author_activity | |
| ): | |
| last_author_activity = comment_date | |
| reference_date = last_author_activity or last_changes_requested | |
| now = datetime.now(timezone.utc) | |
| return (now - reference_date).days |
🤖 Prompt for AI Agents
In `@openwisp_utils/bots/auto_assign/stale_pr_bot.py` around lines 43 - 71, The
code currently calls self.repo.get_commits(sha=pr.head.sha) which returns the
entire reachable history and can be huge; replace that call with
pr.get_commits() to iterate only the commits belonging to the pull request (keep
the existing commit/date logic and last_author_activity variable), and avoid
materializing all commits at once (iterate the PR commits generator instead of
wrapping with list()) to reduce API pagination and rate-limit usage in the
methods like the stale PR age computation used by process_stale_prs.
| elif days_inactive >= self.DAYS_BEFORE_UNASSIGN: | ||
| if not self.has_bot_comment( | ||
| pr, "stale" | ||
| ) and not self.has_bot_comment(pr, "unassigned"): | ||
| if self.mark_pr_stale(pr, days_inactive): | ||
| processed_count += 1 |
There was a problem hiding this comment.
has_bot_comment(pr, "unassigned") on line 313 checks for a marker that is never posted.
No method posts <!-- bot:unassigned -->. The check is inert (always returns False) and can be removed to avoid confusion.
Proposed fix
elif days_inactive >= self.DAYS_BEFORE_UNASSIGN:
- if not self.has_bot_comment(
- pr, "stale"
- ) and not self.has_bot_comment(pr, "unassigned"):
+ if not self.has_bot_comment(pr, "stale"):
if self.mark_pr_stale(pr, days_inactive):
processed_count += 1🤖 Prompt for AI Agents
In `@openwisp_utils/bots/auto_assign/stale_pr_bot.py` around lines 310 - 315, The
check for has_bot_comment(pr, "unassigned") is dead code because no method ever
posts the "<!-- bot:unassigned -->" marker; remove that check from the
conditional in stale_pr_bot.py (the block around DAYS_BEFORE_UNASSIGN and
mark_pr_stale) so the branch simply tests has_bot_comment(pr, "stale") and then
calls mark_pr_stale(pr, days_inactive) as before, leaving DAYS_BEFORE_UNASSIGN,
has_bot_comment, and mark_pr_stale intact.
| elif days_inactive >= self.DAYS_BEFORE_STALE_WARNING: | ||
| if not self.has_bot_comment( | ||
| pr, "reminder" | ||
| ) and not self.has_bot_comment(pr, "stale warning"): | ||
| if self.send_stale_warning(pr, days_inactive): | ||
| processed_count += 1 |
There was a problem hiding this comment.
Bug: marker mismatch — "stale warning" (space) vs "stale_warning" (underscore) causes repeated warnings.
send_stale_warning posts <!-- bot:stale_warning --> (line 250, underscore), but process_stale_prs checks for has_bot_comment(pr, "stale warning") (line 320, space). The marker <!-- bot:stale warning --> will never match <!-- bot:stale_warning -->, so the deduplication fails and the warning will be sent every day the cron runs.
Additionally, has_bot_comment(pr, "reminder") on line 319 looks for <!-- bot:reminder --> which is never posted anywhere — this check is dead code.
🐛 Proposed fix
elif days_inactive >= self.DAYS_BEFORE_STALE_WARNING:
- if not self.has_bot_comment(
- pr, "reminder"
- ) and not self.has_bot_comment(pr, "stale warning"):
+ if not self.has_bot_comment(pr, "stale_warning"):
if self.send_stale_warning(pr, days_inactive):
processed_count += 1🤖 Prompt for AI Agents
In `@openwisp_utils/bots/auto_assign/stale_pr_bot.py` around lines 317 - 322, The
dedupe check in process_stale_prs is using has_bot_comment(pr, "stale warning")
and has_bot_comment(pr, "reminder") which don't match the actual marker posted
by send_stale_warning (<!-- bot:stale_warning -->) and no code ever posts <!--
bot:reminder -->; update process_stale_prs to call has_bot_comment(pr,
"stale_warning") (underscore) instead of "stale warning" and remove the dead
has_bot_comment(pr, "reminder") check (or replace it only if you add a matching
reminder marker elsewhere), ensuring deduplication uses the same marker string
as send_stale_warning.
| def test_has_bot_comment(self): | ||
| bot = StalePRBot() | ||
|
|
||
| mock_pr = Mock() | ||
| mock_comment = Mock() | ||
| mock_comment.user.type = "Bot" | ||
| mock_comment.body = "This is a stale warning" | ||
| mock_pr.get_issue_comments.return_value = [mock_comment] | ||
|
|
||
| result = bot.has_bot_comment(mock_pr, "stale") | ||
| self.assertTrue(result) | ||
|
|
||
| result = bot.has_bot_comment(mock_pr, "closed") | ||
| self.assertFalse(result) |
There was a problem hiding this comment.
Bug: test_has_bot_comment will fail — mock body doesn't contain the HTML marker.
has_bot_comment (in stale_pr_bot.py line 99) searches for <!-- bot:stale --> in comment.body. The mock body is "This is a stale warning", which doesn't contain that marker. The assertion on line 149 (assertTrue) will fail.
🐛 Proposed fix
def test_has_bot_comment(self):
bot = StalePRBot()
mock_pr = Mock()
mock_comment = Mock()
mock_comment.user.type = "Bot"
- mock_comment.body = "This is a stale warning"
+ mock_comment.body = "<!-- bot:stale --> This is a stale warning"
mock_pr.get_issue_comments.return_value = [mock_comment]
result = bot.has_bot_comment(mock_pr, "stale")
self.assertTrue(result)
- result = bot.has_bot_comment(mock_pr, "closed")
+ result = bot.has_bot_comment(mock_pr, "stale_warning")
self.assertFalse(result)🤖 Prompt for AI Agents
In `@openwisp_utils/bots/auto_assign/stale_pr_tests.py` around lines 139 - 152,
The test test_has_bot_comment sets mock_comment.body to "This is a stale
warning" but has_bot_comment (StalePRBot.has_bot_comment) looks for the HTML
marker "<!-- bot:stale -->"; update the test so mock_comment.body includes that
exact marker (e.g., include "<!-- bot:stale --> This is a stale warning") when
calling bot.has_bot_comment(mock_pr, "stale"), and keep a body without the
marker for the "closed" assertion to ensure the True/False expectations are
correct.
374ace8 to
2315ec8
Compare
Eeshu-Yadav
force-pushed
the
issues/571-automate-assigning-unassigning-issues
branch
from
2315ec8 to
62926b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/pr-issue-link.yml:
- Around line 31-33: Replace the direct interpolation echo '${{
toJson(github.event) }}' with an environment variable to avoid shell injection:
define an env key (e.g., EVENT_JSON: ${{ toJson(github.event) }}) at the step or
job level and change the run to write that variable into event.json (use quoted
expansion, e.g., echo "$EVENT_JSON" > event.json) before invoking python -m
openwisp_utils.bots.auto_assign issue_assignment event.json so the payload is
not interpolated into the shell.
In `@openwisp_utils/bots/auto_assign/issue_assignment_bot.py`:
- Around line 246-251: The truncation of linked_issues in
issue_assignment_bot.py is nondeterministic because extract_linked_issues
returns a set-backed list; before slicing linked_issues =
linked_issues[:max_issues] sort the list deterministically (e.g., by issue
identifier or string) so the same subset is chosen each run; update the block
around linked_issues, max_issues (and any call sites using
extract_linked_issues) to perform a stable sort (e.g.,
linked_issues.sort(key=...)) before applying the slice.
🧹 Nitpick comments (9)
.coveragerc (1)
9-9: Blanket exclusion hides bot test coverage.
/*/bots/*excludes both the bot source and the bot test files from coverage measurement. This means test quality for the bot modules will never be tracked. If the intent is to exclude only the bot source (because tests run viaunittest.mockwithout real GitHub calls), consider a narrower pattern or explicitly covering the test files.Alternatively, if bots are tested in a separate CI job, document that rationale so future contributors don't wonder about the coverage gap.
openwisp_utils/bots/auto_assign/__main__.py (2)
19-19: Minor: Use iterable unpacking for list construction (Ruff RUF005).♻️ Proposed fix
- sys.argv = [sys.argv[0]] + sys.argv[2:] + sys.argv = [sys.argv[0], *sys.argv[2:]]Apply the same change on Line 28.
15-38:sys.argvmutation is fragile — consider passing args explicitly.Mutating
sys.argvto forward arguments to sub-modulemain()functions couples the dispatcher tightly to the sub-modules' argument parsing. If any sub-module changes its argv expectations, this breaks silently. A cleaner approach would be to have each sub-module'smain()accept an optional args parameter (likeargparsesupports), but this is a larger refactor and the current approach works for now..github/workflows/issue-assignment.yml (1)
10-13: Consider adding a comment-body filter to reduce unnecessary workflow runs.Every comment on every non-PR issue triggers this workflow, including unrelated discussion. The previous version had
contains()filters on theifcondition. While the Python bot handles filtering internally, each run still incurs checkout + Python setup + pip install overhead. Re-adding a lightweightiffilter for known trigger phrases would save CI minutes.This is optional since correctness is handled by the bot.
openwisp_utils/bots/auto_assign/test_pr_reopen_bot.py (1)
41-56: Consider testing edge cases forreassign_issues_to_author.The test covers the happy path (single
Fixes#123`` reference). It would be valuable to also test:
- PR body with multiple issue references (e.g.,
Fixes#123, Closes#456``)- PR body with no issue references
- Issue that is already assigned
These can be added in a follow-up.
openwisp_utils/bots/auto_assign/test_stale_pr_bot.py (1)
96-118: Fragiledatetimemock — comparisons with realdatetimeobjects may break.Patching the
datetimeclass in the module meanscommit_date > last_changes_requested(stale_pr_bot.py Line 50) is comparing realdatetimeinstances that were passed into the method. This works today because the mock only interceptsdatetime.now(), but if the implementation ever usesdatetime(...)constructors internally, the mock will return aMockinstead. Consider usingfreezegunor patching onlydatetime.nowfor robustness.openwisp_utils/bots/auto_assign/pr_reopen_bot.py (1)
10-33: Significant code duplication betweenPRReopenBot.__init__andPRActivityBot.__init__.Both classes have nearly identical
__init__,load_event_payload, andextract_linked_issuesmethods. Consider extracting a shared base class to reduce maintenance burden.♻️ Sketch of a shared base class
class BaseBotMixin: def __init__(self): self.github_token = os.environ.get("GITHUB_TOKEN") self.repository_name = os.environ.get("REPOSITORY") self.event_name = os.environ.get("GITHUB_EVENT_NAME") self.event_payload = None if self.github_token and self.repository_name: try: self.github = Github(self.github_token) self.repo = self.github.get_repo(self.repository_name) except Exception as e: print(f"Warning: Could not initialize GitHub client: {e}") self.github = None self.repo = None else: self.github = None self.repo = None def load_event_payload(self, event_payload): self.event_payload = event_payloadAlso applies to: 163-180
openwisp_utils/bots/auto_assign/stale_pr_bot.py (2)
282-289:list(self.repo.get_pulls(state="open"))eagerly loads all open PRs.For repositories with many open PRs, this materializes the entire paginated result set at once. Consider iterating the
PaginatedListdirectly rather than converting to a list, and adding a reasonable upper bound to prevent unbounded API calls.♻️ Proposed improvement
- open_prs = list(self.repo.get_pulls(state="open")) - print(f"Found {len(open_prs)} open pull requests") + open_prs = self.repo.get_pulls(state="open") + print(f"Processing open pull requests (total: {open_prs.totalCount})")
148-189:close_stale_prdoesn't include a bot marker in its comment.Both
mark_pr_staleandsend_stale_warninginclude<!-- bot:... -->markers for deduplication, butclose_stale_prdoesn't. While the PR is closed afterward (so it won't be re-processed), adding a marker like<!-- bot:closed -->would be consistent and enable detection if the PR is later reopened.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.coveragerc.github/workflows/issue-assignment.yml.github/workflows/pr-issue-link.yml.github/workflows/pr-reopen-reassign.yml.github/workflows/stale-pr-management.ymlopenwisp_utils/bots/__init__.pyopenwisp_utils/bots/auto_assign/__init__.pyopenwisp_utils/bots/auto_assign/__main__.pyopenwisp_utils/bots/auto_assign/apps.pyopenwisp_utils/bots/auto_assign/issue_assignment_bot.pyopenwisp_utils/bots/auto_assign/pr_reopen_bot.pyopenwisp_utils/bots/auto_assign/stale_pr_bot.pyopenwisp_utils/bots/auto_assign/test_issue_assignment_bot.pyopenwisp_utils/bots/auto_assign/test_pr_reopen_bot.pyopenwisp_utils/bots/auto_assign/test_stale_pr_bot.pysetup.pytests/openwisp2/settings.py
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/pr-reopen-reassign.yml
- setup.py
- .github/workflows/stale-pr-management.yml
🧰 Additional context used
🧬 Code graph analysis (6)
openwisp_utils/bots/auto_assign/test_stale_pr_bot.py (1)
openwisp_utils/bots/auto_assign/stale_pr_bot.py (8)
extract_linked_issues(112-118)get_last_changes_requested(77-93)get_days_since_activity(38-75)unassign_linked_issues(120-146)has_bot_comment(95-110)send_stale_warning(242-280)mark_pr_stale(191-240)close_stale_pr(148-189)
openwisp_utils/bots/auto_assign/test_issue_assignment_bot.py (1)
openwisp_utils/bots/auto_assign/issue_assignment_bot.py (8)
is_assignment_request(39-54)extract_linked_issues(226-232)respond_to_assignment_request(137-224)auto_assign_issues_from_pr(234-291)unassign_issues_from_pr(293-326)handle_issue_comment(328-358)handle_pull_request(360-389)run(391-412)
openwisp_utils/bots/auto_assign/pr_reopen_bot.py (2)
openwisp_utils/bots/auto_assign/issue_assignment_bot.py (4)
load_event_payload(35-37)extract_linked_issues(226-232)run(391-412)main(415-429)openwisp_utils/bots/auto_assign/stale_pr_bot.py (3)
extract_linked_issues(112-118)run(335-350)main(353-356)
openwisp_utils/bots/auto_assign/stale_pr_bot.py (3)
openwisp_utils/bots/auto_assign/issue_assignment_bot.py (3)
extract_linked_issues(226-232)run(391-412)main(415-429)openwisp_utils/bots/auto_assign/pr_reopen_bot.py (5)
extract_linked_issues(39-47)extract_linked_issues(186-194)run(141-160)run(265-284)main(287-309)openwisp_utils/bots/auto_assign/__main__.py (1)
main(6-38)
openwisp_utils/bots/auto_assign/issue_assignment_bot.py (2)
openwisp_utils/releaser/commitizen.py (1)
message(76-88)openwisp_utils/bots/auto_assign/stale_pr_bot.py (1)
extract_linked_issues(112-118)
openwisp_utils/bots/auto_assign/__main__.py (3)
openwisp_utils/bots/auto_assign/issue_assignment_bot.py (1)
main(415-429)openwisp_utils/bots/auto_assign/pr_reopen_bot.py (1)
main(287-309)openwisp_utils/bots/auto_assign/stale_pr_bot.py (1)
main(353-356)
🪛 Ruff (0.14.14)
openwisp_utils/bots/auto_assign/pr_reopen_bot.py
[warning] 21-21: Do not catch blind exception: Exception
(BLE001)
[warning] 84-84: Do not catch blind exception: Exception
(BLE001)
[warning] 90-90: Consider moving this statement to an else block
(TRY300)
[warning] 92-92: Do not catch blind exception: Exception
(BLE001)
[warning] 110-110: Do not catch blind exception: Exception
(BLE001)
[warning] 137-137: Do not catch blind exception: Exception
(BLE001)
[warning] 156-156: Do not catch blind exception: Exception
(BLE001)
[warning] 174-174: Do not catch blind exception: Exception
(BLE001)
[warning] 229-229: Do not catch blind exception: Exception
(BLE001)
[warning] 249-249: Do not catch blind exception: Exception
(BLE001)
[warning] 259-259: Consider moving this statement to an else block
(TRY300)
[warning] 261-261: Do not catch blind exception: Exception
(BLE001)
[warning] 280-280: Do not catch blind exception: Exception
(BLE001)
[warning] 306-306: Do not catch blind exception: Exception
(BLE001)
openwisp_utils/bots/auto_assign/stale_pr_bot.py
[warning] 24-24: Do not catch blind exception: Exception
(BLE001)
[warning] 73-73: Do not catch blind exception: Exception
(BLE001)
[warning] 91-91: Do not catch blind exception: Exception
(BLE001)
[warning] 107-107: Consider moving this statement to an else block
(TRY300)
[warning] 108-108: Do not catch blind exception: Exception
(BLE001)
[warning] 139-139: Do not catch blind exception: Exception
(BLE001)
[warning] 142-142: Consider moving this statement to an else block
(TRY300)
[warning] 144-144: Do not catch blind exception: Exception
(BLE001)
[warning] 185-185: Consider moving this statement to an else block
(TRY300)
[warning] 187-187: Do not catch blind exception: Exception
(BLE001)
[warning] 229-229: Do not catch blind exception: Exception
(BLE001)
[warning] 236-236: Consider moving this statement to an else block
(TRY300)
[warning] 238-238: Do not catch blind exception: Exception
(BLE001)
[warning] 276-276: Consider moving this statement to an else block
(TRY300)
[warning] 278-278: Do not catch blind exception: Exception
(BLE001)
[warning] 326-326: Do not catch blind exception: Exception
(BLE001)
[warning] 332-332: Do not catch blind exception: Exception
(BLE001)
[warning] 345-345: Consider moving this statement to an else block
(TRY300)
[warning] 346-346: Do not catch blind exception: Exception
(BLE001)
openwisp_utils/bots/auto_assign/issue_assignment_bot.py
[warning] 21-21: Do not catch blind exception: Exception
(BLE001)
[warning] 221-221: Consider moving this statement to an else block
(TRY300)
[warning] 222-222: Do not catch blind exception: Exception
(BLE001)
[warning] 283-283: Do not catch blind exception: Exception
(BLE001)
[warning] 288-288: Consider moving this statement to an else block
(TRY300)
[warning] 289-289: Do not catch blind exception: Exception
(BLE001)
[warning] 319-319: Do not catch blind exception: Exception
(BLE001)
[warning] 322-322: Consider moving this statement to an else block
(TRY300)
[warning] 324-324: Do not catch blind exception: Exception
(BLE001)
[warning] 354-354: Consider moving this statement to an else block
(TRY300)
[warning] 356-356: Do not catch blind exception: Exception
(BLE001)
[warning] 385-385: Consider moving this statement to an else block
(TRY300)
[warning] 387-387: Do not catch blind exception: Exception
(BLE001)
[warning] 408-408: Do not catch blind exception: Exception
(BLE001)
[warning] 426-426: Do not catch blind exception: Exception
(BLE001)
openwisp_utils/bots/auto_assign/__main__.py
[warning] 19-19: Consider [sys.argv[0], *sys.argv[2:]] instead of concatenation
Replace with [sys.argv[0], *sys.argv[2:]]
(RUF005)
[warning] 28-28: Consider [sys.argv[0], *sys.argv[2:]] instead of concatenation
Replace with [sys.argv[0], *sys.argv[2:]]
(RUF005)
[warning] 35-35: Consider moving this statement to an else block
(TRY300)
[warning] 36-36: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (9)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.2.0
🔇 Additional comments (18)
openwisp_utils/bots/auto_assign/apps.py (1)
1-7: LGTM!Clean and standard Django
AppConfigdefinition.tests/openwisp2/settings.py (1)
33-33: LGTM!App registration is needed for Django test discovery of the bot module.
.github/workflows/pr-issue-link.yml (1)
3-5: Verifypull_request_targetis intentional here.
pull_request_targetruns the workflow from the base branch with write permissions, which is correct for automation that needs to modify issues. However, be aware thatactions/checkout@v4on Line 16 checks out the base branch, not the PR branch. If the intent is to run the bot code from the PR (e.g., during development), this won't pick up PR changes to the bot. For production use onmaster, this is the expected and safer behavior..github/workflows/issue-assignment.yml (1)
26-34: Good: event data passed via env var avoids script injection.Unlike
pr-issue-link.yml, this workflow correctly usesEVENT_DATAas an environment variable, preventing shell injection from untrusted issue comment bodies.openwisp_utils/bots/auto_assign/test_pr_reopen_bot.py (2)
1-11: LGTM!Good use of conditional imports with
try/exceptto gracefully skip tests when the dependency isn't available, and proper use ofpatch.dictandMockthroughout.
162-183: Good: the non-author test now exercises the correct code path.With the
pull_requestkey present in the payload (addressing the prior review), this test properly reaches the author-check logic and returnsFalsefor a non-author commenter.openwisp_utils/bots/auto_assign/test_stale_pr_bot.py (2)
1-11: LGTM — well-structured test setup with graceful import handling.The
try/except ImportErrorguard andsetUp/tearDownpattern with environment patching and GitHub mock is clean and consistent across the test suite.
154-207: Good coverage for warning, stale-marking, and closing lifecycle.These tests properly verify the comment creation, label manipulation, state change, and unassignment side-effects for each stage of the stale PR lifecycle.
openwisp_utils/bots/auto_assign/test_issue_assignment_bot.py (3)
37-48: LGTM — initialization tests cover both success and missing-env paths.
50-96: LGTM — thorough positive/negative cases withsubTestfor assignment requests and linked-issue extraction.
121-169: LGTM — good coverage for auto-assign success, skip-already-assigned, and skip-PR-reference paths.openwisp_utils/bots/auto_assign/pr_reopen_bot.py (1)
287-313: LGTM —main()correctly dispatches based onGITHUB_EVENT_NAME.The previous
sys.argv[0]issue has been properly addressed by using the environment variable for bot selection.openwisp_utils/bots/auto_assign/stale_pr_bot.py (2)
12-36: LGTM — initialization with configurable thresholds.The
DAYS_BEFORE_STALE_WARNING,DAYS_BEFORE_UNASSIGN, andDAYS_BEFORE_CLOSEconstants are clearly defined and the GitHub client setup follows the same pattern as other bots.
95-110: LGTM —has_bot_commentnow uses structured HTML markers for unambiguous matching.The
<!-- bot:{comment_type} -->pattern properly distinguishes different comment types, addressing the earlier substring-matching concern.openwisp_utils/bots/auto_assign/issue_assignment_bot.py (4)
10-33: LGTM — standard initialization with env-var guarded GitHub client setup.
137-224: Well-designed response with issue-type-aware linking instructions.The
respond_to_assignment_requestmethod provides contextual guidance (bug vs. feature vs. generic) and references contributing guidelines. Good user experience.
234-291: LGTM —auto_assign_issues_from_prproperly handles edge cases.Correctly skips PR references, already-assigned issues, and 404 errors with informative logging.
391-412: No changes needed — the event name is correct.The code correctly uses
"pull_request_target"which matches the GitHub Actions workflows configured in.github/workflows/pr-reopen-reassign.ymland.github/workflows/pr-issue-link.yml. The tests explicitly expect this event name, and it is used consistently across the codebase (also inpr_reopen_bot.py).pull_request_targetis the intended event for handling pull requests from forks with restricted permissions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| run: | | ||
| echo '${{ toJson(github.event) }}' > event.json | ||
| python -m openwisp_utils.bots.auto_assign issue_assignment event.json |
There was a problem hiding this comment.
Critical: Script injection via pull_request_target event payload.
${{ toJson(github.event) }} is interpolated directly into the shell script. Since pull_request_target runs on events from external PRs, an attacker can craft a PR title/body containing ' plus arbitrary shell commands, escaping the single-quoted string and executing code with GITHUB_TOKEN permissions.
Use an environment variable instead — exactly as issue-assignment.yml already does:
🔒 Proposed fix
- name: Run issue assignment bot
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
REPOSITORY: ${{ github.repository }}
GITHUB_EVENT_NAME: ${{ github.event_name }}
+ EVENT_DATA: ${{ toJson(github.event) }}
run: |
- echo '${{ toJson(github.event) }}' > event.json
+ echo "$EVENT_DATA" > event.json
python -m openwisp_utils.bots.auto_assign issue_assignment event.json🤖 Prompt for AI Agents
In @.github/workflows/pr-issue-link.yml around lines 31 - 33, Replace the direct
interpolation echo '${{ toJson(github.event) }}' with an environment variable to
avoid shell injection: define an env key (e.g., EVENT_JSON: ${{
toJson(github.event) }}) at the step or job level and change the run to write
that variable into event.json (use quoted expansion, e.g., echo "$EVENT_JSON" >
event.json) before invoking python -m openwisp_utils.bots.auto_assign
issue_assignment event.json so the payload is not interpolated into the shell.
21de263 to
d4b2948
Compare
I've created four GitHub Actions workflow files to automate issue assignment/unassignment. Created Workflows: 1. issue-assignment.yml - Auto-response to 'assign me' requests 2. pr-issue-link.yml - Auto-assign on PR 3. stale-pr-management.yml - Stale PR management (tracks contributor inactivity only) 4. pr-reopen-reassign.yml - Handle contributor follow-up Fixes openwisp#571
Eeshu-Yadav
force-pushed
the
issues/571-automate-assigning-unassigning-issues
branch
from
d4b2948 to
a3c16b3
Compare
3 participants
Checklist
Created Workflows
1.
issue-assignment.ymlAuto-response to "assign me" requests
2.
pr-issue-link.ymlAuto-assign on PR
Fixes #123,Closes #123, etc.)3.
stale-pr-management.ymlStale PR management (tracks contributor inactivity only)
stalelabel, unassigns linked issues4.
pr-reopen-reassign.ymlHandle contributor follow-up
stalelabelFixes #571