Skip to content

Always allow direct review requests #2014

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 29, 2025
Merged

Always allow direct review requests #2014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3f7ff18
Keep information about review candidates longer
Kobzol May 25, 2025
b6e9fab
Allow running multiple checks on an assignment context
Kobzol May 25, 2025
4bb6f24
Always allow requesting users directly
Kobzol May 26, 2025
3775fcd
Post a warning for suppressed reviewer errors
Kobzol May 26, 2025
97f989a
Rename `Reviewer` to `ReviewerSelection`
Kobzol May 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 94 additions & 39 deletions src/handlers/assign.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ pub(super) async fn handle_input(
if event.issue.assignees.is_empty() {
let (assignee, from_comment) =
determine_assignee(ctx, assign_command, event, config, &diff).await?;
if assignee.as_deref() == Some(GHOST_ACCOUNT) {
if assignee.as_ref().map(|r| r.name.as_str()) == Some(GHOST_ACCOUNT) {
// "ghost" is GitHub's placeholder account for deleted accounts.
// It is used here as a convenient way to prevent assignment. This
// is typically used for rollups or experiments where you don't
Expand All @@ -205,7 +205,7 @@ pub(super) async fn handle_input(
.await
{
let who_text = match &assignee {
Some(assignee) => WELCOME_WITH_REVIEWER.replace("{assignee}", assignee),
Some(assignee) => WELCOME_WITH_REVIEWER.replace("{assignee}", &assignee.name),
None => WELCOME_WITHOUT_REVIEWER.to_string(),
};
let mut welcome = NEW_USER_WELCOME_MESSAGE.replace("{who}", &who_text);
Expand All @@ -221,7 +221,7 @@ pub(super) async fn handle_input(
} else if !from_comment {
let welcome = match &assignee {
Some(assignee) => RETURNING_USER_WELCOME_MESSAGE
.replace("{assignee}", assignee)
.replace("{assignee}", &assignee.name)
.replace("{bot}", &ctx.username),
None => RETURNING_USER_WELCOME_MESSAGE_NO_REVIEWER
.replace("{author}", &event.issue.user.login),
Expand Down Expand Up @@ -268,37 +268,38 @@ async fn set_assignee(
ctx: &Context,
issue: &Issue,
github: &GithubClient,
username: &str,
reviewer: &ReviewerSelection,
) -> anyhow::Result<()> {
let mut db = ctx.db.get().await;
let mut state: IssueData<'_, Reviewers> =
IssueData::load(&mut db, &issue, PREVIOUS_REVIEWERS_KEY).await?;

// Don't re-assign if already assigned, e.g. on comment edit
if issue.contain_assignee(&username) {
if issue.contain_assignee(&reviewer.name) {
log::trace!(
"ignoring assign PR {} to {}, already assigned",
issue.global_id(),
username,
reviewer.name,
);
return Ok(());
}
if let Err(err) = issue.set_assignee(github, &username).await {
if let Err(err) = issue.set_assignee(github, &reviewer.name).await {
log::warn!(
"failed to set assignee of PR {} to {}: {:?}",
issue.global_id(),
username,
reviewer.name,
err
);
if let Err(e) = issue
.post_comment(
github,
&format!(
"Failed to set assignee to `{username}`: {err}\n\
"Failed to set assignee to `{}`: {err}\n\
\n\
> **Note**: Only org members with at least the repository \"read\" role, \
users with write permissions, or people who have commented on the PR may \
be assigned."
be assigned.",
reviewer.name
),
)
.await
Expand All @@ -308,9 +309,30 @@ async fn set_assignee(
}
}

// Record the reviewer in the database
// If an error was suppressed, post a warning on the PR.
if let Some(suppressed_error) = &reviewer.suppressed_error {
let warning = match suppressed_error {
FindReviewerError::ReviewerOffRotation { username } => Some(format!(
r"`{username}` is not on the review rotation at the moment.
They may take a while to respond.
"
)),
FindReviewerError::ReviewerAtMaxCapacity { username } => Some(format!(
"`{username}` is currently at their maximum review capacity.
They may take a while to respond."
)),
_ => None,
};
if let Some(warning) = warning {
if let Err(err) = issue.post_comment(&ctx.github, &warning).await {
// This is a best-effort warning, do not do anything apart from logging if it fails
log::warn!("failed to post reviewer warning comment: {err}");
}
}
}

state.data.names.insert(username.to_lowercase());
// Record the reviewer in the database
state.data.names.insert(reviewer.name.to_lowercase());
state.save().await?;
Ok(())
}
Expand All @@ -330,7 +352,7 @@ async fn determine_assignee(
event: &IssuesEvent,
config: &AssignConfig,
diff: &[FileDiff],
) -> anyhow::Result<(Option<String>, bool)> {
) -> anyhow::Result<(Option<ReviewerSelection>, bool)> {
let mut db_client = ctx.db.get().await;
let teams = crate::team_data::teams(&ctx.github).await?;
if let Some(name) = assign_command {
Expand Down Expand Up @@ -693,7 +715,7 @@ fn get_team_name<'a>(teams: &Teams, issue: &Issue, name: &'a str) -> Option<&'a
teams.teams.get(team_name).map(|_| team_name)
}

#[derive(PartialEq, Debug)]
#[derive(PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
enum FindReviewerError {
/// User specified something like `r? foo/bar` where that team name could
/// not be found.
Expand All @@ -715,7 +737,7 @@ enum FindReviewerError {
ReviewerPreviouslyAssigned { username: String },
/// Data required for assignment could not be loaded from the DB.
DatabaseError(String),
/// The reviewer has too many PRs alreayd assigned.
/// The reviewer has too many PRs already assigned.
ReviewerAtMaxCapacity { username: String },
}

Expand Down Expand Up @@ -781,6 +803,24 @@ Please select a different reviewer.",
}
}

/// Reviewer that was found to be eligible as a result of `r? <...>`.
/// In some cases, a reviewer selection error might have been suppressed.
/// We store it here to allow sending a comment with a warning about the suppressed error.
#[derive(PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
struct ReviewerSelection {
name: String,
suppressed_error: Option<FindReviewerError>,
}

impl ReviewerSelection {
fn from_name(name: String) -> Self {
Self {
name,
suppressed_error: None,
}
}
}

/// Finds a reviewer to assign to a PR.
///
/// The `names` is a list of candidate reviewers `r?`, such as `compiler` or
Expand All @@ -794,11 +834,11 @@ async fn find_reviewer_from_names(
config: &AssignConfig,
issue: &Issue,
names: &[String],
) -> Result<String, FindReviewerError> {
) -> Result<ReviewerSelection, FindReviewerError> {
// Fast path for self-assign, which is always allowed.
if let [name] = names {
if is_self_assign(&name, &issue.user.login) {
return Ok(name.clone());
return Ok(ReviewerSelection::from_name(name.clone()));
}
}

Expand Down Expand Up @@ -827,26 +867,25 @@ async fn find_reviewer_from_names(
// sure they are really worth the effort.

log::info!(
"[#{}] Initial unfiltered list of candidates: {:?}",
"[#{}] Filtered list of candidates: {:?}",
issue.number,
candidates
);

// Return unfiltered list of candidates
// Select a random reviewer from the filtered list
Ok(candidates
.into_iter()
.choose(&mut rand::thread_rng())
.expect("candidate_reviewers_from_names should return at least one entry")
.to_string())
.expect("candidate_reviewers_from_names should return at least one entry"))
}

#[derive(Eq, PartialEq, Hash)]
#[derive(Eq, PartialEq, Hash, Debug)]
struct ReviewerCandidate {
name: String,
origin: ReviewerCandidateOrigin,
}

#[derive(Eq, PartialEq, Hash, Copy, Clone)]
#[derive(Eq, PartialEq, Hash, Copy, Clone, Debug)]
enum ReviewerCandidateOrigin {
/// This reviewer was directly requested for a review.
Direct,
Expand Down Expand Up @@ -962,7 +1001,7 @@ async fn candidate_reviewers_from_names<'a>(
config: &'a AssignConfig,
issue: &Issue,
names: &'a [String],
) -> Result<HashSet<String>, FindReviewerError> {
) -> Result<HashSet<ReviewerSelection>, FindReviewerError> {
// Step 1: expand teams and groups into candidate names
let expanded = expand_teams_and_groups(teams, issue, config, names)?;
let expanded_count = expanded.len();
Expand All @@ -976,7 +1015,7 @@ async fn candidate_reviewers_from_names<'a>(

// Set of candidate usernames to choose from.
// We go through each expanded candidate and store either success or an error for them.
let mut candidates: Vec<Result<String, FindReviewerError>> = Vec::new();
let mut candidates: Vec<Result<ReviewerCandidate, FindReviewerError>> = Vec::new();
let previous_reviewer_names = get_previous_reviewer_names(db, issue).await;

// Step 2: pre-filter candidates based on checks that we can perform quickly
Expand Down Expand Up @@ -1022,7 +1061,7 @@ async fn candidate_reviewers_from_names<'a>(
if let Some(error_reason) = reason {
candidates.push(Err(error_reason));
} else {
candidates.push(Ok(reviewer_candidate.name));
candidates.push(Ok(reviewer_candidate));
}
}
assert_eq!(candidates.len(), expanded_count);
Expand All @@ -1031,7 +1070,7 @@ async fn candidate_reviewers_from_names<'a>(
// Step 3: gather potential usernames to form a DB query for review preferences
let usernames: Vec<String> = candidates
.iter()
.filter_map(|res| res.as_deref().ok().map(|s| s.to_string()))
.filter_map(|res| res.as_ref().ok().map(|s| s.name.to_string()))
.collect();
let usernames: Vec<&str> = usernames.iter().map(|s| s.as_str()).collect();
let review_prefs = get_review_prefs_batch(db, &usernames)
Expand All @@ -1044,35 +1083,40 @@ async fn candidate_reviewers_from_names<'a>(
// Step 4: check review preferences
candidates = candidates
.into_iter()
.map(|username| {
.map(|candidate| {
// Only consider candidates that did not have an earlier error
let username = username?;
let candidate = candidate?;
let username = &candidate.name;

// If no review prefs were found, we assume the default unlimited
// review capacity and being on rotation.
let Some(review_prefs) = review_prefs.get(username.as_str()) else {
return Ok(username);
return Ok(candidate);
};
if let Some(capacity) = review_prefs.max_assigned_prs {
let assigned_prs = workqueue.assigned_pr_count(review_prefs.user_id as UserId);
// Is the reviewer at max capacity?
if (assigned_prs as i32) >= capacity {
return Err(FindReviewerError::ReviewerAtMaxCapacity { username });
return Err(FindReviewerError::ReviewerAtMaxCapacity {
username: username.clone(),
});
}
}
if review_prefs.rotation_mode == RotationMode::OffRotation {
return Err(FindReviewerError::ReviewerOffRotation { username });
return Err(FindReviewerError::ReviewerOffRotation {
username: username.clone(),
});
}

return Ok(username);
return Ok(candidate);
})
.collect();
}
assert_eq!(candidates.len(), expanded_count);

let valid_candidates: HashSet<&str> = candidates
.iter()
.filter_map(|res| res.as_deref().ok())
.filter_map(|res| res.as_ref().ok().map(|c| c.name.as_str()))
.collect();

log::debug!(
Expand All @@ -1083,13 +1127,24 @@ async fn candidate_reviewers_from_names<'a>(
);

if valid_candidates.is_empty() {
// If we requested a single user for a review, we return a concrete error message
// describing why they couldn't be assigned.
if is_single_user {
Err(candidates
// If we requested a single user for a review, we may suppress some errors.
// Check what error we got here.
let error = candidates
.pop()
.unwrap()
.expect_err("valid_candidates is empty, so this should be an error"))
.expect_err("valid_candidates is empty, so this should be an error");
let username = match &error {
// If the reviewer is at capacity or off rotation, allow them to be requested,
// but store the suppressed error.
FindReviewerError::ReviewerOffRotation { username }
| FindReviewerError::ReviewerAtMaxCapacity { username } => username,
_ => return Err(error),
};
Ok(HashSet::from([ReviewerSelection {
name: username.to_string(),
suppressed_error: Some(error),
}]))
} else {
// If it was a request for a team or a group, and no one is available, simply
// return `NoReviewer`.
Expand All @@ -1105,7 +1160,7 @@ async fn candidate_reviewers_from_names<'a>(
} else {
Ok(valid_candidates
.into_iter()
.map(|s| s.to_string())
.map(|s| ReviewerSelection::from_name(s.to_string()))
.collect())
}
}
Expand Down
Loading