Bug 2003867 - reviewer rotation seems to always assign patches to the same reviewers within the group

[dklawren]

Summary

Reviewer rotation seems to always assign patches to the same reviewers within the group.

How the Rotation Algorithm Works

1. Load the rotation group's member list, sorted by Phabricator user ID
2. Query DB for the last selected reviewer for this rotation group
3. Rotate the member list so the last reviewer is at the front
4. Loop through members and pick the first eligible candidate:
   • Skip the last reviewer (line 442)
   • Skip the revision author (line 450)
   • Skip anyone with block_reviews enabled (line 452)
   • Skip anyone who can't see the associated bug (line 451)
5. Store the selected reviewer as the new "last reviewer" in DB

Root Cause

The main selection loop (lines 438-458) excludes:

1. The last reviewer (unconditionally, line 442)
2. The revision author (line 450)
3. Anyone with block_reviews = on (line 452)
4. Anyone who can't see the bug (line 451)

With a 3-person group, if:

• One member is the author
• One member is the last reviewer (skipped unconditionally)
• One member has block_reviews enabled

All three members are excluded, and the error at line 469 fires:

"An available rotation reviewer was not found from the group webdriver-reviewers-rotation for this revision."

Proposed Fix

The last reviewer should be reconsidered as a fallback when all other candidates are exhausted. Being the last reviewer doesn't disqualify someone — it just means they shouldn't be the first choice.

After the main loop fails to find a candidate, try the last reviewer as a fallback before giving up. The "skip last reviewer" rule is a preference for rotation, not a hard exclusion.

[Copilot]

Pull request overview

This PR fixes reviewer-rotation behavior in the PhabBugz integration by scoping rotation state per author and adding a fallback pass to prevent “no available reviewer found” failures for small/unavailable groups.

Changes:

• Track last-selected reviewer per (project_phid, author_phid) instead of globally per project.
• Add a fallback selection pass that allows re-using the last reviewer when no other eligible reviewer exists.
• Update DB schema + migration to add author_phid and a unique index, and switch rotation updates to an atomic upsert.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
extensions/PhabBugz/lib/Util.pm	Implements per-author rotation lookups/updates and adds fallback reviewer selection logic; switches DB write to upsert.
extensions/PhabBugz/Extension.pm	Extends phab_reviewer_rotation schema with author_phid, adds unique index, and provides an upgrade migration.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[cgsheeh]

This PR still implements per-author rotation. Per-author rotation is not necessary, and was only suggested due to a misunderstanding of the actual issue (the incorrect rotation location of the previous reviewer). If I misunderstand and we do need per-author rotation, please elaborate on why.

The commit message still contains the incorrect assessment of the problem, please update it to be accurate with the current implementation.

[cgsheeh]

LGTM. Would be great if we could write a unit test showing how the lists are rotated.

[dklawren]

LGTM. Would be great if we could write a unit test showing how the lists are rotated.

I had Claude whip something up and it looked good so including it here.