Skip to content

Pick reviewers more fairly: own PRs don't count as load, ties go to the longest wait - #45

Open
willson556 wants to merge 1 commit into
mainfrom
fair-reviewer-pick
Open

willson556 wants to merge 1 commit into
mainfrom
fair-reviewer-pick

Conversation

@willson556

@willson556 willson556 commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

Makes the shared least-loaded reviewer pick (ReviewConfig/pick.js) fairer. AssignReviewers uses it to pick a new PR's assignees, and ReviewSLA uses it when it replaces someone who is out of office.

In the first days of Proposal 1, the rotation picks were uneven:

  • One reviewer got 6 of the 9 rotation picks.
  • Another was picked once.

Two things caused it:

  1. Self-assignment counted as load. Load is the number of open, ready PRs someone is assigned to, and it counted PRs they wrote. A reviewer who assigns themselves to their own PRs looked like the busiest person on the team and was passed over. Now a PR doesn't count toward its own author's load.
  2. Ties were broken by PR number. PRs merge quickly through the merge queue, so most people have 0 open assigned PRs most of the time, and most picks are ties. PR number % number tied only evens out over many PRs, and it doesn't remember who went recently. Ties now go to whoever was assigned to someone else's PR longest ago, with anyone not assigned recently first. Only then does PR number decide.

When each person was last assigned comes from assignment events on the most recently updated PRs in the 20 most recently pushed repos. Like load, it's read from live PR data rather than search, so back-to-back picks see each other.

I first added this to the load query, but the combined query timed out against the org, so it's a second, smaller query. If it fails, the job warns and breaks ties by PR number as before. If the load query fails, nothing changes from today.

AssignReviewers' log line now includes when the person was last assigned, for example picked X from embeddedreviewers (0 open assigned PRs, last assigned 2026-10-01).

Testing:

  • node --test AssignReviewers/ ReviewConfig/ ReviewSLA/: 87 cases, 3 of them new. They cover a PR not counting toward its author's load, ties going to the longest wait (with someone not assigned recently first), and a failed last-assigned query falling back to load and PR number. Under the old rules, all three would pick someone else.
  • Ran both queries against the org with my token: the load and last-assigned times for the review team match what the PRs show.

🤖 Generated with Claude Code

@willson556
willson556 changed the base branch from review-sla-remind to graphite-base/45 October 2, 2026 22:59
@staflsystemsci

staflsystemsci Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review SLA

Assignee Owes Due
fcheein First response Mon Oct 5, 13:01 PT

Due times are in business hours, 10:00 to 17:00 Pacific: 4 h for a PR under 250 added lines, 7 h otherwise, and 4 h for a re-review. Mid-stack, an assignee owes a response on the lowest PR they haven't approved. Dashboard

@graphite-app
graphite-app Bot changed the base branch from graphite-base/45 to main October 2, 2026 23:01
…he longest wait

Two changes to the shared least-loaded pick, used by AssignReviewers and by
ReviewSLA's out-of-office reassignment:

- A PR doesn't count toward its author's load. Someone who assigns
  themselves to their own PRs looked busier than everyone else and was
  passed over.
- PRs merge quickly, so most people sit at 0 open assigned PRs and most
  picks are ties. Ties now go to whoever was assigned to someone else's PR
  longest ago, and only then rotate by PR number, which doesn't remember who
  went recently.

When each person was last assigned comes from a second, smaller query of
recent assignment events; one combined query timed out. If it fails, ties
fall back to PR number as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants