[ci:fix] Refined reusable autoassign workflow - #623
Conversation
Avoid suggesting to look in openwisp-utils, which is a special case.
📝 WalkthroughWalkthroughThis PR modifies GitHub Actions bot handlers to change return value semantics, treating previously unhandled or skipped scenarios as successfully processed (returning Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py:
- Line 294: The test currently only asserts that bot.handle_issue_comment()
returns truthy for skip paths; update it to also assert no side-effect calls
were made (e.g., ensure repo.get_issue() and any issue comment creation methods
were not invoked). Specifically, in the test surrounding assert
bot.handle_issue_comment() add negative assertions on the mocked
repository/issue methods used by the bot (for example, assert_not_called on
repo.get_issue and on the issue comment creation mock), and apply the same
additional no-side-effect assertions to the other similar assertion at the later
test location (the one around line 307) so both skip-paths confirm handled=True
and no repo/issue interactions occurred.
In @.github/actions/bot-autoassign/tests/test_pr_reopen_bot.py:
- Line 163: The tests only assert that bot.handle_contributor_activity() returns
truthy for skip branches but don't verify no side-effects occurred; update each
skip-path test (the ones calling bot.handle_contributor_activity()) to also
assert that the mocked side-effect methods (the repository/github client methods
used by the bot to reassign PRs, add/remove labels, or post comments) were not
called—i.e., after calling bot.handle_contributor_activity() assert
mock_reassign.assert_not_called(), mock_add_label.assert_not_called(),
mock_remove_label.assert_not_called(), and
mock_create_comment.assert_not_called() (use the actual mock names used in these
tests) so each skip branch verifies no reassignment/label/comment side effects
occurred.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b2f0eec3-f80c-4f0a-ae1d-8d6c8cb18c9e
📒 Files selected for processing (9)
.github/actions/bot-autoassign/issue_assignment_bot.py.github/actions/bot-autoassign/pr_reopen_bot.py.github/actions/bot-autoassign/stale_pr_bot.py.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py.github/actions/bot-autoassign/tests/test_pr_reopen_bot.py.github/actions/bot-autoassign/tests/test_stale_pr_bot.py.github/workflows/bot-autoassign-pr-issue-link.yml.github/workflows/reusable-bot-autoassign.ymldocs/developer/reusable-github-utils.rst
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: pushpitkamboj
Repo: openwisp/openwisp-utils PR: 584
File: .github/workflows/reusable-bot-changelog.yml:49-49
Timestamp: 2026-03-05T09:38:10.320Z
Learning: In openwisp-utils, PR title prefixes are strictly limited to `[feature]`, `[fix]`, and `[change]` (exact bracketed tags, no scoping/sub-types). The regex `^\[(feature|fix|change)\]` in `.github/workflows/reusable-bot-changelog.yml` is intentional and correct — scoped variants like `[feature/bots]` are not valid and should not be matched.
Learnt from: nemesifier
Repo: openwisp/openwisp-utils PR: 584
File: .github/workflows/reusable-bot-changelog.yml:22-24
Timestamp: 2026-03-05T14:23:55.528Z
Learning: In `.github/workflows/reusable-bot-changelog.yml`, the maintainer (nemesifier) has explicitly decided that `github.event.review.author_association == 'COLLABORATOR'` should be allowed (alongside `OWNER` and `MEMBER`) to trigger the changelog bot workflow. The rationale is that the workflow is non-destructive and only posts a PR comment — it cannot make code changes. Do not flag `COLLABORATOR` as a security issue for this workflow.
📚 Learning: 2026-03-05T14:23:55.528Z
Learnt from: nemesifier
Repo: openwisp/openwisp-utils PR: 584
File: .github/workflows/reusable-bot-changelog.yml:22-24
Timestamp: 2026-03-05T14:23:55.528Z
Learning: In `.github/workflows/reusable-bot-changelog.yml`, the maintainer (nemesifier) has explicitly decided that `github.event.review.author_association == 'COLLABORATOR'` should be allowed (alongside `OWNER` and `MEMBER`) to trigger the changelog bot workflow. The rationale is that the workflow is non-destructive and only posts a PR comment — it cannot make code changes. Do not flag `COLLABORATOR` as a security issue for this workflow.
Applied to files:
.github/workflows/bot-autoassign-pr-issue-link.yml.github/actions/bot-autoassign/issue_assignment_bot.py.github/workflows/reusable-bot-autoassign.ymldocs/developer/reusable-github-utils.rst.github/actions/bot-autoassign/pr_reopen_bot.py
📚 Learning: 2026-03-05T09:38:10.320Z
Learnt from: pushpitkamboj
Repo: openwisp/openwisp-utils PR: 584
File: .github/workflows/reusable-bot-changelog.yml:49-49
Timestamp: 2026-03-05T09:38:10.320Z
Learning: In openwisp-utils, PR title prefixes are strictly limited to `[feature]`, `[fix]`, and `[change]` (exact bracketed tags, no scoping/sub-types). The regex `^\[(feature|fix|change)\]` in `.github/workflows/reusable-bot-changelog.yml` is intentional and correct — scoped variants like `[feature/bots]` are not valid and should not be matched.
Applied to files:
.github/workflows/bot-autoassign-pr-issue-link.ymldocs/developer/reusable-github-utils.rst
📚 Learning: 2026-03-05T20:55:29.968Z
Learnt from: nemesifier
Repo: openwisp/openwisp-utils PR: 0
File: :0-0
Timestamp: 2026-03-05T20:55:29.968Z
Learning: For the openwisp/openwisp-utils repository (and the broader openwisp org), the maintainers have explicitly decided to use mutable version tags (e.g. `v2`, `v6`) for GitHub Actions and to use `ref: master` for internal script checkouts. Do NOT flag unpinned action versions or `master`/version-tag usage as security issues in any openwisp-utils workflow file. The maintainers are aware of the trade-offs and are intentionally fine with this approach.
Applied to files:
docs/developer/reusable-github-utils.rst
📚 Learning: 2026-03-14T20:44:14.555Z
Learnt from: CR
Repo: openwisp/openwisp-utils PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T20:44:14.555Z
Learning: Changes: Update tests to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py
🔇 Additional comments (11)
.github/actions/bot-autoassign/stale_pr_bot.py (1)
147-147: Handled-state return for already-closed PRs looks correct.Treating this branch as successfully handled keeps bot semantics consistent with the new workflow behavior.
.github/actions/bot-autoassign/tests/test_stale_pr_bot.py (1)
234-234: Test expectation update is correct.This assertion now matches the intended “already closed = handled” behavior.
.github/workflows/bot-autoassign-pr-issue-link.yml (1)
18-18: Merged-PR guard condition is correctly implemented.The job now skips merged-closed events while preserving required execution paths.
.github/actions/bot-autoassign/pr_reopen_bot.py (1)
79-79: Skip-path and unsupported-event handling is now consistent.These return-value updates align with the new “handled unless exception” contract.
Also applies to: 92-92, 116-116, 120-120, 124-124, 172-172
.github/actions/bot-autoassign/tests/test_pr_reopen_bot.py (1)
111-111: Unsupported-event expectations are correctly updated.These assertions match the new handled/consumed return semantics.
Also applies to: 202-202
.github/actions/bot-autoassign/issue_assignment_bot.py (1)
276-276: Return semantics and merged-close handling look correct.This is consistent with the new policy: skip/unsupported branches are handled, and merged PR closure no longer triggers unassignment.
Also applies to: 288-288, 311-315, 317-317, 334-334
.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py (1)
376-391: Behavioral test updates are solid.The merged-close regression guard and handled=True expectations align well with the updated bot contract.
Also applies to: 404-404, 450-450
.github/workflows/reusable-bot-autoassign.yml (1)
30-31: This comment should be withdrawn. The openwisp project maintainers have explicitly decided to use mutable refs likemasterin workflows and are aware of the trade-offs involved. Additionally, the suggested fix usinggithub.workflow_shadoes not work as intended—that variable points to the caller workflow's SHA, not the called workflow's revision, so it would not achieve the stated reproducibility goal.docs/developer/reusable-github-utils.rst (3)
89-99: Good clarification on reusable workflow permission inheritance.The new note is accurate and prevents common misconfiguration when adopting reusable workflows.
132-151: Merged-PR guard is documented correctly.The
ifcondition on Line 145 correctly skips mergedclosedPRs while still allowing other relevant actions.
153-219: PR reopen/stale PR templates are well-structured and consistent.The split jobs, dispatch explanation, and secret wiring are clear and actionable for downstream repos.
| } | ||
| ) | ||
| assert not bot.handle_issue_comment() | ||
| assert bot.handle_issue_comment() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add no-side-effect assertions for skip paths.
Since these now return handled=True, please also assert they do not perform assignment-response side effects (e.g., no repo.get_issue / no issue comment creation).
Based on learnings: Changes: Update tests to cover non-trivial changes and ensure proper validation of modified behavior.
Also applies to: 307-307
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py at line
294, The test currently only asserts that bot.handle_issue_comment() returns
truthy for skip paths; update it to also assert no side-effect calls were made
(e.g., ensure repo.get_issue() and any issue comment creation methods were not
invoked). Specifically, in the test surrounding assert
bot.handle_issue_comment() add negative assertions on the mocked
repository/issue methods used by the bot (for example, assert_not_called on
repo.get_issue and on the issue comment creation mock), and apply the same
additional no-side-effect assertions to the other similar assertion at the later
test location (the one around line 307) so both skip-paths confirm handled=True
and no repo/issue interactions occurred.
| mock_pr.user.login = "testuser" | ||
| bot_env["repo"].get_pull.return_value = mock_pr | ||
| assert not bot.handle_contributor_activity() | ||
| assert bot.handle_contributor_activity() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen skip-path tests with side-effect assertions.
These tests currently assert truthy return only; please also assert no reassignment/label/comment side effects occurred in each skip branch.
✅ Suggested test hardening
def test_handle_contributor_activity_not_author(self, bot_env):
...
bot_env["repo"].get_pull.return_value = mock_pr
assert bot.handle_contributor_activity()
+ mock_pr.remove_from_labels.assert_not_called()
+ mock_pr.create_issue_comment.assert_not_called()
def test_handle_contributor_activity_pr_not_stale(self, bot_env):
...
bot_env["repo"].get_pull.return_value = mock_pr
assert bot.handle_contributor_activity()
+ mock_pr.remove_from_labels.assert_not_called()
+ mock_pr.create_issue_comment.assert_not_called()
def test_handle_contributor_activity_not_pr(self, bot_env):
...
assert bot.handle_contributor_activity()
+ bot_env["repo"].get_pull.assert_not_called()Also applies to: 180-180, 193-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/actions/bot-autoassign/tests/test_pr_reopen_bot.py at line 163, The
tests only assert that bot.handle_contributor_activity() returns truthy for skip
branches but don't verify no side-effects occurred; update each skip-path test
(the ones calling bot.handle_contributor_activity()) to also assert that the
mocked side-effect methods (the repository/github client methods used by the bot
to reassign PRs, add/remove labels, or post comments) were not called—i.e.,
after calling bot.handle_contributor_activity() assert
mock_reassign.assert_not_called(), mock_add_label.assert_not_called(),
mock_remove_label.assert_not_called(), and
mock_create_comment.assert_not_called() (use the actual mock names used in these
tests) so each skip branch verifies no reassignment/label/comment side effects
occurred.
Continuation of #613, I couldn't push there so I opened a new PR.
CC @Eeshu-Yadav.
I also addressed the issue of avoid triggering the bot when PRs are merged.