[bug] Fixed auto-assign bot claiming success on silent rejection#652
[bug] Fixed auto-assign bot claiming success on silent rejection#652Eeshu-Yadav wants to merge 2 commits intoopenwisp:masterfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 42 minutes and 33 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds bot-command support and assignment verification to the issue-assignment GitHub Action. The bot now recognizes Sequence Diagram(s)sequenceDiagram
actor User as PR Author
participant GitHub_API as GitHub API
participant Bot as IssueAssignmentBot
participant Repo as Repository
User->>GitHub_API: Opens PR referencing issue `#123`
GitHub_API-->>Bot: Webhook: PR opened
Bot->>Repo: extract linked issues from PR body
Repo-->>Bot: returns issue `#123`
alt Issue is open
Bot->>GitHub_API: add_to_assignees(issue `#123`, pr_author)
GitHub_API-->>Bot: 200 OK (may silently ignore)
Bot->>GitHub_API: re-fetch issue `#123` assignees
GitHub_API-->>Bot: assignees list
alt pr_author in assignees
Bot->>GitHub_API: post success comment
else pr_author not in assignees
Bot->>GitHub_API: post fallback explanatory comment
end
else Issue is closed
Bot->>Bot: skip assignment
end
sequenceDiagram
actor User as Issue Commenter
participant GitHub_API as GitHub API
participant Bot as IssueAssignmentBot
participant Repo as Repository
User->>GitHub_API: Posts comment "@<BOT_USERNAME> assign" on issue `#123`
GitHub_API-->>Bot: Webhook: issue_comment created
Bot->>Bot: ignore if action != created or comment is bot-authored
Bot->>Bot: detect bot assign command
alt command detected
Bot->>Repo: find_open_pr_for_issue(commenter, issue `#123`)
Repo-->>Bot: PR found or none
alt PR found and open
Bot->>GitHub_API: add_to_assignees(issue `#123`, commenter)
GitHub_API-->>Bot: 200 OK (should succeed now)
Bot->>GitHub_API: re-fetch issue `#123` assignees
GitHub_API-->>Bot: assignees list
Bot->>GitHub_API: post success or fallback based on verification
else no PR found
Bot->>GitHub_API: post "no matching open PR" message
end
else no command
Bot->>Bot: proceed with regular comment handling
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
coderabbitai
Bot
added
bug
github_actions
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental review: All fix implementations for issue #649 are properly in place. The PR correctly implements the fix for issue #649 with all components properly integrated. Summary of Changes
Implementation Highlights
Files Reviewed (6 files)
Reviewed by kimi-k2.5-0127 · 441,525 tokens |
Eeshu-Yadav
force-pushed
the
issues/649-fix-bot-assignment-silent-failure
branch
from
5c36afe to
5913cf6
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a long-standing behavior in the auto-assign GitHub Action bot where it could post a success comment even when GitHub silently rejected the assignment (common for non-collaborators who haven’t commented yet). It adds assignment verification and a self-serve fallback path via an explicit @openwisp-companion assign command.
Changes:
- Verify that
add_to_assigneesactually took effect (with a short retry) before posting an “assigned” confirmation comment. - Add a bot-mention command flow (
@openwisp-companion assign) that assigns the commenter if they have an open PR that closes the issue. - Wire bot username via
BOT_USERNAMEenv var (workflow input for reusable workflow + explicit env in local workflows) and expand test coverage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/reusable-bot-autoassign.yml |
Adds bot_username workflow input and exports it as BOT_USERNAME for bot scripts. |
.github/workflows/bot-autoassign-pr-issue-link.yml |
Exports BOT_USERNAME for PR-triggered auto-assignment workflow. |
.github/workflows/bot-autoassign-issue.yml |
Exports BOT_USERNAME for issue-comment-triggered workflow. |
.github/actions/bot-autoassign/utils.py |
Adds strict linked-issue extraction mode and helper to find an author’s open PR that closes a given issue. |
.github/actions/bot-autoassign/issue_assignment_bot.py |
Implements assignment verification + fallback messaging + bot assign command handling and bot-comment filtering. |
.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py |
Adds/updates tests for strict extraction, silent rejection handling, verification retry behavior, and bot command flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🤖 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/issue_assignment_bot.py:
- Around line 226-248: The verification routine _assignment_succeeded
incorrectly preserves last_error across retries, causing a successful refetch
that shows the user is not assigned to still return None; clear last_error when
a refetch succeeds (i.e., inside the try block after calling self.repo.get_issue
and before checking assignees) so that a successful API response leads to a
normal False return when the user isn't assigned; update the code around
last_error, get_issue, and the _get_assignee_logins/_user_in checks to reset
last_error on successful fetch.
In @.github/actions/bot-autoassign/utils.py:
- Around line 50-68: The lookup in find_open_pr_for_issue only matches
closing-keyword references because it calls extract_linked_issues(pr.body or "",
strict=True); change this to accept non-closing references by calling
extract_linked_issues(pr.body or "", strict=False) (or removing the strict flag
if default is non-strict) so PRs that say "Related to `#123`" will be considered
in the fallback assignment lookup.
🪄 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: b70d1954-f171-4427-83df-62c35bfff121
📒 Files selected for processing (6)
.github/actions/bot-autoassign/issue_assignment_bot.py.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py.github/actions/bot-autoassign/utils.py.github/workflows/bot-autoassign-issue.yml.github/workflows/bot-autoassign-pr-issue-link.yml.github/workflows/reusable-bot-autoassign.yml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Agent
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | 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.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
🧠 Learnings (7)
📓 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.
Learnt from: stktyagi
Repo: openwisp/openwisp-utils PR: 631
File: .github/workflows/bot-ci-failure.yml:71-74
Timestamp: 2026-03-24T16:40:15.783Z
Learning: In openwisp/openwisp-utils reusable-bot-ci-failure.yml, the auto-retry step uses a custom GitHub App token (generated from APP_ID and PRIVATE_KEY secrets) passed to the gh CLI via the GH_TOKEN environment variable. This token is NOT subject to the workflow's `permissions` block (which only restricts GITHUB_TOKEN). Therefore, having `actions: read` in the reusable workflow's permissions block does NOT break the auto-retry `gh api -X POST` call — it authenticates with the App's own permissions. Do not flag this as a permissions issue.
📚 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-issue.yml.github/workflows/bot-autoassign-pr-issue-link.yml.github/workflows/reusable-bot-autoassign.yml
📚 Learning: 2026-03-24T16:40:15.783Z
Learnt from: stktyagi
Repo: openwisp/openwisp-utils PR: 631
File: .github/workflows/bot-ci-failure.yml:71-74
Timestamp: 2026-03-24T16:40:15.783Z
Learning: In openwisp/openwisp-utils reusable-bot-ci-failure.yml, the auto-retry step uses a custom GitHub App token (generated from APP_ID and PRIVATE_KEY secrets) passed to the gh CLI via the GH_TOKEN environment variable. This token is NOT subject to the workflow's `permissions` block (which only restricts GITHUB_TOKEN). Therefore, having `actions: read` in the reusable workflow's permissions block does NOT break the auto-retry `gh api -X POST` call — it authenticates with the App's own permissions. Do not flag this as a permissions issue.
Applied to files:
.github/workflows/bot-autoassign-issue.yml.github/workflows/bot-autoassign-pr-issue-link.yml.github/workflows/reusable-bot-autoassign.yml
📚 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/actions/bot-autoassign/utils.py
📚 Learning: 2026-03-05T09:59:22.581Z
Learnt from: pushpitkamboj
Repo: openwisp/openwisp-utils PR: 584
File: .github/actions/bot-changelog-generator/generate_changelog.py:356-364
Timestamp: 2026-03-05T09:59:22.581Z
Learning: In `.github/actions/bot-changelog-generator/generate_changelog.py`, the `validate_changelog_output` function's purpose is to act as an output safety filter — ensuring no sensitive information or arbitrary LLM-generated text gets posted as a PR comment. It checks that the output starts with a valid tag ([feature]/[fix]/[change]) and contains a correctly structured PR reference pattern. It is NOT intended to strictly validate that the referenced PR number/URL matches the current PR.
Applied to files:
.github/actions/bot-autoassign/utils.py
📚 Learning: 2026-03-14T20:44:14.568Z
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.568Z
Learning: Bug Fixes: Ensure the test is deterministic and not flaky - flag tests that depend on timing, sleeps, specific timezones, system time, randomness without fixed seed, race conditions, concurrency timing, network access, external services, filesystem state, environment-specific configuration, execution order, shared global state, hardcoded ports, or unawaited async operations
Applied to files:
.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py
📚 Learning: 2026-03-14T20:44:14.568Z
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.568Z
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 (4)
.github/workflows/bot-autoassign-issue.yml (1)
43-43: LGTM — bot username is passed to the issue-comment bot.This keeps command detection aligned with the configured companion app username.
.github/workflows/bot-autoassign-pr-issue-link.yml (1)
44-44: LGTM — username configuration is available to the PR-link bot.The bot can now use the same mention/login value across PR and issue-comment flows.
.github/workflows/reusable-bot-autoassign.yml (1)
10-14: LGTM — reusable callers can override the bot username safely.The default preserves current behavior while exposing the value needed by command parsing.
Also applies to: 53-53
.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py (1)
27-27: LGTM — retry tests remain deterministic.Patching
time.sleepavoids slow or flaky retry-path tests. Based on learnings, ensure tests do not depend on timing or sleeps.
Commit Message Format FailureHello @Eeshu-Yadav, The CI failed because the commit message does not follow the required format. Fix: |
Eeshu-Yadav
force-pushed
the
issues/649-fix-bot-assignment-silent-failure
branch
from
5913cf6 to
deabf9e
Compare
Commit Message Format FailureHello @Eeshu-Yadav, The CI failed because the commit message does not follow the required format. Fix: Here's an example of the correct format: |
…wisp#649 Updated the issue assignment bot to verify GitHub's response before reporting success. Previously, the bot would log a successful assignment even when GitHub silently rejected it (e.g., when the target user lacked repository permissions), leaving issues unassigned without any visible error. The bot now checks the assignees returned in the API response and fails loudly when the intended assignee is missing, so the workflow surfaces the real outcome. Fixes openwisp#649
Eeshu-Yadav
force-pushed
the
issues/649-fix-bot-assignment-silent-failure
branch
from
deabf9e to
c7e2694
Compare
|
coverage: 97.529%. remained the same — Eeshu-Yadav:issues/649-fix-bot-assignment-silent-failure into openwisp:master |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
| issue.create_comment( | ||
| f"Sorry @{commenter}, GitHub still did not allow" | ||
| " this assignment. A maintainer will need to assign" | ||
| " this issue manually." |
There was a problem hiding this comment.
in what case/cases would this happen? A comment that summarizes this would be good.
There was a problem hiding this comment.
Added a short comment — this hits when the commenter has satisfied GitHub's "must have commented" rule but the assignment still fails (e.g. a repo-level permission block or an API outage).
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
last_error not cleared after a successful fetch in _assignment_succeeded
issue_assignment_bot.py:_assignment_succeeded
The loop runs up to two attempts. If attempt 0 raises GithubException (last_error is set),
then attempt 1 calls get_issue successfully but the user is not yet in the assignee list, the
loop exits with last_error is not None. The trailing guard then returns None (silent, no
comment posted) instead of False (assignment verifiably failed, post the fallback message). The
fix is to reset last_error = None immediately after self.repo.get_issue(issue_number) returns
without raising.
Non-GithubException errors escape _assignment_succeeded's contract
issue_assignment_bot.py:_assignment_succeeded
The method's contract states it returns None when verification itself errors so the caller stays
silent. Only GithubException is caught; lower-level network or serialization errors bubble up to
the outer except Exception in the callers, which return False for the whole operation rather
than staying silent. Catch Exception (or at minimum add a catch-all except Exception after
GithubException) to honour the stated contract.
strict=True breaks the fallback flow for "Related to" PRs
utils.py:find_open_pr_for_issue / issue_assignment_bot.py:handle_bot_assign_request
auto_assign_issues_from_pr uses the default (non-strict) extract_linked_issues, so a PR with
Related to #123 triggers auto-assignment of issue 123. If that assignment is silently rejected,
_build_silent_failure_message is posted, telling the contributor to comment
@openwisp-companion assign. When they do, handle_bot_assign_request calls
find_open_pr_for_issue with strict=True, which excludes "Related to" links, and the bot
replies "I could not find an open PR by you". The user followed the bot's own instructions and
still cannot get assigned.
The call site in handle_bot_assign_request should pass strict=False (or rely on the default)
to stay consistent with the auto-assign path.
unassign_linked_issues_helper uses case-sensitive comparison inconsistency
utils.py:unassign_linked_issues_helper
This PR introduces _user_in for case-insensitive login comparison and applies it throughout
IssueAssignmentBot. unassign_linked_issues_helper still does pr_author in current_assignees
with a plain string comparison. GitHub logins are case-insensitive, so if the casing returned by
the API differs from what was stored, the unassign step silently does nothing. The helper should
use the same lowercase-both-sides logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/utils.py:
- Around line 54-75: The current find_open_pr_for_issue function uses
repo.get_pulls and then client-side filters by author which can miss older PRs
due to the 100-PR cap; change it to query server-side via the GitHub Search API
(e.g., use Github.search_issues or repo._requester.requestJsonAndCheck with a
query like "repo:OWNER/REPO is:pr is:open author:AUTHOR
linked:issue:ISSUE_NUMBER") so results are filtered by author on the server and
you won't be limited by the 100 most-recent pulls; if you prefer to keep the
scan approach, at minimum increase or make max_prs configurable and document the
tradeoff in the find_open_pr_for_issue docstring, and ensure you still handle
GithubException the same way.
🪄 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: 7ce69fb3-ccab-4ebf-9ca9-fd110ad3bc1e
📒 Files selected for processing (6)
.github/actions/bot-autoassign/issue_assignment_bot.py.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py.github/actions/bot-autoassign/utils.py.github/workflows/bot-autoassign-issue.yml.github/workflows/bot-autoassign-pr-issue-link.yml.github/workflows/reusable-bot-autoassign.yml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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-24T16:40:15.783Z
Learnt from: stktyagi
Repo: openwisp/openwisp-utils PR: 631
File: .github/workflows/bot-ci-failure.yml:71-74
Timestamp: 2026-03-24T16:40:15.783Z
Learning: In openwisp/openwisp-utils reusable-bot-ci-failure.yml, the auto-retry step uses a custom GitHub App token (generated from APP_ID and PRIVATE_KEY secrets) passed to the gh CLI via the GH_TOKEN environment variable. This token is NOT subject to the workflow's `permissions` block (which only restricts GITHUB_TOKEN). Therefore, having `actions: read` in the reusable workflow's permissions block does NOT break the auto-retry `gh api -X POST` call — it authenticates with the App's own permissions. Do not flag this as a permissions issue.
Applied to files:
.github/workflows/bot-autoassign-issue.yml.github/workflows/reusable-bot-autoassign.yml.github/workflows/bot-autoassign-pr-issue-link.yml
📚 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-issue.yml.github/workflows/reusable-bot-autoassign.yml.github/workflows/bot-autoassign-pr-issue-link.yml
📚 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/actions/bot-autoassign/utils.py.github/workflows/bot-autoassign-pr-issue-link.yml
📚 Learning: 2026-02-10T20:38:27.593Z
Learnt from: nemesifier
Repo: openwisp/openwisp-utils PR: 593
File: openwisp_utils/releaser/commitizen.py:5-9
Timestamp: 2026-02-10T20:38:27.593Z
Learning: In openwisp-utils commitizen implementation, the `_TITLE_ISSUE_EXTRACT_RE` pattern intentionally matches any space-preceded `#<number>` in commit titles without anchoring to the end. This is acceptable because 99.9% of such patterns are actual issue references, and rare edge cases (like version numbers) will be handled manually if they occur.
Applied to files:
.github/actions/bot-autoassign/utils.py
📚 Learning: 2026-02-06T20:46:32.980Z
Learnt from: nemesifier
Repo: openwisp/openwisp-utils PR: 560
File: openwisp_utils/commitizen/openwisp.py:9-25
Timestamp: 2026-02-06T20:46:32.980Z
Learning: In openwisp-utils, the commit message prefix "chores" (plural) is intentionally used instead of "chore" (singular) in the Commitizen ALLOWED_PREFIXES configuration.
Applied to files:
.github/actions/bot-autoassign/issue_assignment_bot.py
📚 Learning: 2026-03-14T20:44:14.568Z
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.568Z
Learning: Bug Fixes: Ensure the test is deterministic and not flaky - flag tests that depend on timing, sleeps, specific timezones, system time, randomness without fixed seed, race conditions, concurrency timing, network access, external services, filesystem state, environment-specific configuration, execution order, shared global state, hardcoded ports, or unawaited async operations
Applied to files:
.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py
🔇 Additional comments (15)
.github/actions/bot-autoassign/utils.py (2)
6-8: LGTM on the case-insensitive membership helper.Clean, null-safe, and reusable across the bot for assignee comparisons. Nicely replaces the previous exact-match checks.
88-91: LGTM — case-insensitive assignee match in the unassign path.Good catch aligning this with GitHub's case-insensitive login semantics; fixes the regression where a PR whose assignee login was stored with different casing than
pr_authorwould skip the unassign..github/workflows/bot-autoassign-issue.yml (1)
43-43: LGTM — explicitBOT_USERNAMEwiring.Keeps the workflow explicit and matches the bot's default; makes it trivial to override per-fork if the GitHub App is renamed.
.github/workflows/bot-autoassign-pr-issue-link.yml (1)
44-44: LGTM — consistentBOT_USERNAMEinjection on the PR-target workflow.Matches the sibling issue-comment workflow; keeps bot-identity configuration centralized at the workflow layer.
.github/workflows/reusable-bot-autoassign.yml (1)
10-14: LGTM — reusable workflow exposesbot_usernamewith a safe default.Non-breaking addition (optional input) and correctly forwarded into the bot environment. Downstream callers can override without code changes if the bot identity ever changes.
Also applies to: 53-53
.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py (5)
28-28: LGTM — sleep patched to no-op at fixture level.Keeps the new verification-retry tests deterministic and fast without leaking into other test modules.
159-192: LGTM — reusable mock builders.
_make_issue_with_assignment'sadd_to_assignees.side_effectis a nice touch: it mirrors GitHub's behavior (assignee list grows on successful add), so verification-positive tests exercise the real post-assignment code path._make_bot_assign_issuenicely parameterizes state/pull_request/assignees for thehandle_bot_assign_requestscenarios.
283-301: LGTM — regression test for the errored-then-recovered verification path.This pins down the exact bug previously flagged (verification errors leaking into the retry outcome) — ensuring that a transient error on the first attempt followed by a successful refetch that shows the user is not assigned correctly yields the fallback comment (not silent suppression). Good defensive coverage.
605-635: LGTM — thorough bot-command detection coverage.Negative cases (
assignee,assigning,assigns,assignment) validate the\bassign\bword boundary, and@someone-else assign/assign@openwisp-companion`` validate both the anchored mention and the lookbehind. Case-insensitivity also covered.
827-844: Excellent regression: fallback message cannot self-trigger.This is an important guard — the
_cannot_auto_assign_messagebody literally contains`@openwisp-companion assign`, which would matchis_bot_assign_command. Proving that_is_bot_commentshort-circuits before command detection prevents infinite self-trigger loops..github/actions/bot-autoassign/issue_assignment_bot.py (5)
22-26: LGTM — robust bot-mention detection.The negative lookbehind
(?<![\w-])correctly prevents false matches on patterns likeuser@openwisp-companion(email-like), and the terminal\bafterassignrejectsassignee/assigning/assignment(validated in the parametrized tests). Case-insensitive match mirrors GitHub's login semantics.
217-232: LGTM — verification semantics are correct and match the three-state contract.The
errored_allflag is flipped toFalseas soon as anyget_issuesucceeds, so the sequence "error → success showing not-assigned" correctly returnsFalse(silent-rejection confirmed) rather thanNone(can't tell). That was the key bug flagged in prior review, and it is handled cleanly. TheNonereturn for the all-errors case is also correctly propagated by callers (no comment posted), preserving idempotency on transient outages.
246-309: LGTM — bot-command handler covers the intended state machine.Good layering: cross-repo/PR-target/closed/already-assigned checks short-circuit with clear logs, and the three verification outcomes each get appropriate treatment (confirmation, "maintainer must assign manually", silent skip on verification failure). The "still silently rejected" branch usefully distinguishes this failure from the initial auto-assign failure, giving the maintainer a clear escalation signal.
Minor nit (non-blocking): a comment that runs
@<bot> assignon a closed issue currently gets no feedback (silentreturn True). Consider posting a one-liner like "This issue is closed — a maintainer will need to reopen it before assignment" if you want symmetry with the other user-visible branches. Not required.
333-377: LGTM — verification-gated auto-assign with correct fallback routing.Three things worth calling out as well-done:
- Closed-issue skip at line 333 closes a latent gap (prior code would have attempted assignment on stale closed issues since
get_valid_linked_issuesdoesn't filter by state).- Confirmation comment is now strictly gated on
verified is True, fixing the "claims success on silent rejection" bug that motivated this PR.- The
verified is Nonebranch correctly does not post the fallback (avoids misleading users during transient GH outages where we genuinely can't tell).
400-410: LGTM — tight filtering prevents self-triggering and edit abuse.The defensive union of
user.type == "Bot",performed_via_github_app, and login-matches-bot cleanly rules out self-triggered loops (the fallback message text itself contains@<bot> assign). Ignoring non-createdactions also neutralizes a subtle attack vector where a user could edit an innocuous comment to contain the bot command post-hoc and avoid review — good hardening.Also applies to: 417-420, 432-436
Eeshu-Yadav
force-pushed
the
issues/649-fix-bot-assignment-silent-failure
branch
from
c6b34a9 to
ab4033d
Compare
Eeshu-Yadav
force-pushed
the
issues/649-fix-bot-assignment-silent-failure
branch
from
ab4033d to
ca62c02
Compare
…p#649 Reset verification-error tracking after a successful refetch so a verified "user not assigned" result posts the fallback instead of staying silent. Broaden exception handling in _assignment_succeeded to honour the "stay silent on verification error" contract for non- GithubException failures. Drop strict=True in find_open_pr_for_issue so the bot-command flow accepts the same relate-to references that trigger auto-assign. Extract user_in_logins helper and use it in unassign_linked_issues_helper so GitHub login casing differences no longer cause silent no-ops. Rename _build_silent_failure_message to _cannot_auto_assign_message, point contributors at the explicit @openwisp-companion assign command, and move the GithubException import to the test module top. Fixes openwisp#649
Eeshu-Yadav
force-pushed
the
issues/649-fix-bot-assignment-silent-failure
branch
from
ca62c02 to
d4e57e5
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@Eeshu-Yadav great progress!
The testing confirms the full flow works end-to-end, excellent! 👍
See my comments below to polish the code a bit more so we can merge.
Misleading variable name in _assignment_succeeded
issue_assignment_bot.py:218-232
errored_all starts as True and is set to False the first time a get_issue call returns successfully. A reader has to hold that inversion in their head throughout the loop. The intended meaning is "has at least one fetch succeeded", so the variable reads backwards relative to its name. A clearer formulation:
def _assignment_succeeded(self, issue_number, user):
had_successful_fetch = False
for attempt in range(2):
try:
updated_issue = self.repo.get_issue(issue_number)
had_successful_fetch = True
if user_in_logins(user, self._get_assignee_logins(updated_issue)):
return True
except Exception as e:
print(...)
if attempt == 0:
time.sleep(VERIFICATION_RETRY_DELAY_SECONDS)
return False if had_successful_fetch else NoneMethod name _assignment_succeeded implies a boolean but can return None
issue_assignment_bot.py:217
The method returns True, False, or None. A name ending in "succeeded" implies a bool, which is what callers see in if verified is True / elif verified is False / else. Something like _verify_assignment signals that the result must be compared explicitly rather than used as a plain truthiness check. Not urgent, but will trip up anyone reading the code cold.
Is None intentional here? If yes, please add a brief docstring explaining what's the difference between None and False.
find_open_pr_for_issue only catches GithubException
utils.py:68
except GithubException as e:
print(f"GitHub API error searching open PRs by {author}: {e}")
return None_assignment_succeeded catches Exception for consistency with the "stay silent on unexpected errors" contract. find_open_pr_for_issue does not. If a lower-level network error (e.g. requests.exceptions.ConnectionError) is raised during the search, it propagates to the outer except Exception in handle_bot_assign_request, which logs "Error handling bot assign command" and returns False — and the user gets no comment at all. Catching Exception here would let the function return None gracefully and keep the caller's logic intact.
extract_linked_issues is called twice on the same PR body
issue_assignment_bot.py:318 and utils.py:30
auto_assign_issues_from_pr calls extract_linked_issues(pr_body) to check for an early exit and to log the rate-limit warning, then immediately calls get_valid_linked_issues(…, pr_body) which calls extract_linked_issues again internally. The PR body is typically small so the overhead is negligible, but two calls on the same input is fragile: anyone who later makes extract_linked_issues stateful or expensive will have to trace this dependency. The simpler fix is to call it once, store the result, and pass the list into get_valid_linked_issues (or skip the outer call and use get_valid_linked_issues's result for the no-issues check).
Redundant f-string split in auto_assign_issues_from_pr
issue_assignment_bot.py:381
print(f"Error processing issue" f" #{issue_number}: {e}")Two adjacent f-strings with no reason to be separate. Should be one:
print(f"Error processing issue #{issue_number}: {e}")Missing test: fetch succeeds on attempt 0 (no assignee), fails on attempt 1
The test suite covers:
- both fetches fail →
None(test_verification_error_stays_silent) - fetch 0 fails, fetch 1 succeeds with empty assignees →
False(test_verification_recovers_then_reports_failure) - fetch 0 stale, fetch 1 fresh with assignee →
True(test_verification_retries_on_transient_lag)
The untested case is: fetch 0 succeeds (user not in assignees, errored_all = False), fetch 1 raises. The return is False because had_successful_fetch (or not errored_all) is True. This is the correct behaviour — attempt 0 gave reliable evidence the user is not assigned — but there is no test for it.
reusable-bot-autoassign.yml still points to the testing fork
reusable-bot-autoassign.yml:35-37
# TEMPORARY: testing #649 fix — revert to openwisp/openwisp-utils@master before merge.
repository: Eeshu-Yadav/openwisp-utils
ref: issues/649-fix-bot-assignment-silent-failureThis must be reverted to repository: openwisp/openwisp-utils / ref: master before the PR is merged. Any downstream repository that calls the reusable workflow will continue pulling from the contributor's fork after merge, which means their bots will silently break the moment the fork diverges or is deleted.
Checklist
Reference to Existing Issue
Closes #649
Verify assignment actually took effect before posting the confirmation comment. On silent rejection (non-collaborators who have not commented on the issue), post an honest fallback message instructing the user to trigger assignment via the "@openwisp-companion assign" bot command.
Testing on
bot-testing-groundWorkflows on
openwisp/bot-testing-ground@mastertemporarily point at this branch(commit
7b24383)so the silent-rejection and recovery paths can be exercised end-to-end by a
non-org contributor.
@openwisp-companion assignrecovery@openwisp-companion assignrecovery (second run)