From f4f6be4f76f242ffb5d9dde0b77f48ceb1e26c16 Mon Sep 17 00:00:00 2001 From: reddevilmidzy Date: Mon, 26 Jan 2026 20:29:33 +0900 Subject: [PATCH 1/4] Warn about unknown reviewers when approving --- src/bors/comment.rs | 8 ++++ src/bors/handlers/review.rs | 80 +++++++++++++++++++++++++++++++++++-- 2 files changed, 84 insertions(+), 4 deletions(-) diff --git a/src/bors/comment.rs b/src/bors/comment.rs index ec0efab6..6b200db1 100644 --- a/src/bors/comment.rs +++ b/src/bors/comment.rs @@ -291,6 +291,7 @@ pub fn approved_comment( repo: &GithubRepoName, commit_sha: &CommitSha, reviewer: &str, + unknown_reviewers: Option, tree_state: TreeState, ) -> Comment { let approve_emoji = if is_holiday_season() { @@ -305,6 +306,13 @@ It is now in the [queue]({web_url}/queue/{}) for this repository. ", repo.name() ); + if let Some(unknown) = unknown_reviewers { + writeln!( + comment, + "\n:warning: The following reviewer(s) could not be found: `{unknown}`" + ) + .unwrap(); + } if let TreeState::Closed { priority, source } = tree_state { let tree_emoji = if is_holiday_season() { "christmas_tree" diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index 816f2486..f89d41ab 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -68,10 +68,15 @@ pub(super) async fn command_approve( } } - let approver = match approver { - Approver::Myself => author.username.clone(), - Approver::Specified(approver) => normalize_approvers(approver), + let (approver, unknown_reviewers) = match approver { + Approver::Myself => (author.username.clone(), None), + Approver::Specified(approver) => { + let normalized = normalize_approvers(approver); + let unknown = check_unknown_reviewers(&repo_state, &normalized).await; + (normalized, unknown) + } }; + let approval_info = ApprovalInfo { approver: approver.clone(), sha: pr.github.head.sha.to_string(), @@ -83,7 +88,16 @@ pub(super) async fn command_approve( let priority = priority.or(pr.db.priority.map(|p| p as u32)); merge_queue_tx.notify().await?; - notify_of_approval(ctx, &db, &repo_state, pr, priority, approver.as_str()).await?; + notify_of_approval( + ctx, + &db, + &repo_state, + pr, + priority, + approver.as_str(), + unknown_reviewers, + ) + .await?; handle_label_trigger(&repo_state, pr.github, LabelTrigger::Approved).await } @@ -97,6 +111,40 @@ fn normalize_approvers(approvers: &str) -> String { .join(",") } +/// Check if the specified reviewers exist as GitHub users or teams. +/// Returns comma-separated string of unknown reviewer names, or None if all exist. +async fn check_unknown_reviewers(repo_state: &RepositoryState, reviewers: &str) -> Option { + let mut unknown_reviewers = Vec::new(); + + for reviewer in reviewers.split(',') { + let user_exists = repo_state + .client + .client() + .users(reviewer) + .profile() + .await + .is_ok(); + if !user_exists { + let team_exists = repo_state + .client + .client() + .teams(repo_state.repository().owner()) + .get(reviewer) + .await + .is_ok(); + if !team_exists { + unknown_reviewers.push(reviewer); + } + } + } + + if unknown_reviewers.is_empty() { + None + } else { + Some(unknown_reviewers.join(",")) + } +} + /// Keywords that will prevent an approval if they appear in the PR's title. /// They are checked in a case-insensitive manner. const WIP_KEYWORDS: &[&str] = &["wip", "[do not merge]"]; @@ -424,6 +472,7 @@ async fn notify_of_approval( pr: PullRequestData<'_>, priority: Option, approver: &str, + unknown_reviewers: Option, ) -> anyhow::Result<()> { let mut tree_state = ctx .db @@ -451,6 +500,7 @@ async fn notify_of_approval( repo.repository(), &pr.github.head.sha, approver, + unknown_reviewers, tree_state, ), db, @@ -592,6 +642,28 @@ approved = ["+approved"] .await; } + #[sqlx::test] + async fn approve_with_unknown_reviewer(pool: sqlx::PgPool) { + run_test(pool, async |ctx: &mut BorsTester| { + // Approve with a user that doesn't exist + ctx.post_comment("@bors r=nonexistent-user").await?; + insta::assert_snapshot!( + ctx.get_next_comment_text(()).await?, + @r###" + :pushpin: Commit pr-1-sha has been approved by `nonexistent-user` + + It is now in the [queue](https://bors-test.com/queue/borstest) for this repository. + + :warning: The following reviewer(s) could not be found: `nonexistent-user` + "### + ); + + ctx.pr(()).await.expect_approved_by("nonexistent-user"); + Ok(()) + }) + .await; + } + #[sqlx::test] async fn insufficient_permission_approve(pool: sqlx::PgPool) { run_test(pool, async |ctx: &mut BorsTester| { From 7ba453317a804bbcfe0b5bf5ccafa079127a48ff Mon Sep 17 00:00:00 2001 From: reddevilmidzy Date: Tue, 27 Jan 2026 18:56:37 +0900 Subject: [PATCH 2/4] Use Vec for approvers and unknowns --- src/bors/comment.rs | 7 ++++--- src/bors/handlers/review.rs | 29 +++++++++++++---------------- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/bors/comment.rs b/src/bors/comment.rs index 6b200db1..863a71f9 100644 --- a/src/bors/comment.rs +++ b/src/bors/comment.rs @@ -291,7 +291,7 @@ pub fn approved_comment( repo: &GithubRepoName, commit_sha: &CommitSha, reviewer: &str, - unknown_reviewers: Option, + unknown_reviewers: Vec, tree_state: TreeState, ) -> Comment { let approve_emoji = if is_holiday_season() { @@ -306,10 +306,11 @@ It is now in the [queue]({web_url}/queue/{}) for this repository. ", repo.name() ); - if let Some(unknown) = unknown_reviewers { + if !unknown_reviewers.is_empty() { writeln!( comment, - "\n:warning: The following reviewer(s) could not be found: `{unknown}`" + "\n:warning: The following reviewer(s) could not be found: `{}`", + unknown_reviewers.join(", ") ) .unwrap(); } diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index f89d41ab..1c50d691 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -69,11 +69,11 @@ pub(super) async fn command_approve( } let (approver, unknown_reviewers) = match approver { - Approver::Myself => (author.username.clone(), None), + Approver::Myself => (author.username.clone(), Vec::new()), Approver::Specified(approver) => { let normalized = normalize_approvers(approver); let unknown = check_unknown_reviewers(&repo_state, &normalized).await; - (normalized, unknown) + (normalized.join(","), unknown) } }; @@ -103,20 +103,22 @@ pub(super) async fn command_approve( /// Normalize approvers (given after @bors r=) by removing leading @, possibly from multiple /// usernames split by a comma. -fn normalize_approvers(approvers: &str) -> String { +fn normalize_approvers(approvers: &str) -> Vec { approvers .split(',') - .map(|approver| approver.trim_start_matches('@')) - .collect::>() - .join(",") + .map(|approver| approver.trim_start_matches('@').to_string()) + .collect::>() } /// Check if the specified reviewers exist as GitHub users or teams. /// Returns comma-separated string of unknown reviewer names, or None if all exist. -async fn check_unknown_reviewers(repo_state: &RepositoryState, reviewers: &str) -> Option { +async fn check_unknown_reviewers( + repo_state: &RepositoryState, + reviewers: &Vec, +) -> Vec { let mut unknown_reviewers = Vec::new(); - for reviewer in reviewers.split(',') { + for reviewer in reviewers { let user_exists = repo_state .client .client() @@ -133,16 +135,11 @@ async fn check_unknown_reviewers(repo_state: &RepositoryState, reviewers: &str) .await .is_ok(); if !team_exists { - unknown_reviewers.push(reviewer); + unknown_reviewers.push(reviewer.clone()); } } } - - if unknown_reviewers.is_empty() { - None - } else { - Some(unknown_reviewers.join(",")) - } + unknown_reviewers } /// Keywords that will prevent an approval if they appear in the PR's title. @@ -472,7 +469,7 @@ async fn notify_of_approval( pr: PullRequestData<'_>, priority: Option, approver: &str, - unknown_reviewers: Option, + unknown_reviewers: Vec, ) -> anyhow::Result<()> { let mut tree_state = ctx .db From e09542885ae99955d291696a9fcc583e7d48167c Mon Sep 17 00:00:00 2001 From: reddevilmidzy Date: Tue, 27 Jan 2026 21:10:49 +0900 Subject: [PATCH 3/4] Use Team API data to validate reviewer existence --- src/bors/handlers/review.rs | 31 +++++++------------------------ src/permissions.rs | 30 +++++++++++++++++++++--------- 2 files changed, 28 insertions(+), 33 deletions(-) diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index 1c50d691..b61266a0 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -114,32 +114,15 @@ fn normalize_approvers(approvers: &str) -> Vec { /// Returns comma-separated string of unknown reviewer names, or None if all exist. async fn check_unknown_reviewers( repo_state: &RepositoryState, - reviewers: &Vec, + reviewers: &[String], ) -> Vec { - let mut unknown_reviewers = Vec::new(); + let permission = repo_state.permissions.load(); - for reviewer in reviewers { - let user_exists = repo_state - .client - .client() - .users(reviewer) - .profile() - .await - .is_ok(); - if !user_exists { - let team_exists = repo_state - .client - .client() - .teams(repo_state.repository().owner()) - .get(reviewer) - .await - .is_ok(); - if !team_exists { - unknown_reviewers.push(reviewer.clone()); - } - } - } - unknown_reviewers + reviewers + .iter() + .filter(|reviewer| !permission.has_reviewer(reviewer)) + .cloned() + .collect() } /// Keywords that will prevent an approval if they appear in the PR's title. diff --git a/src/permissions.rs b/src/permissions.rs index 55891c00..f0c7fe90 100644 --- a/src/permissions.rs +++ b/src/permissions.rs @@ -24,13 +24,19 @@ impl fmt::Display for PermissionType { pub struct UserPermissions { review_users: HashSet, + review_usernames: HashSet, try_users: HashSet, } impl UserPermissions { - pub fn new(review_users: HashSet, try_users: HashSet) -> Self { + pub fn new( + review_users: HashSet, + review_usernames: HashSet, + try_users: HashSet, + ) -> Self { Self { review_users, + review_usernames, try_users, } } @@ -41,11 +47,16 @@ impl UserPermissions { PermissionType::Try => self.try_users.contains(&user_id), } } + + pub fn has_reviewer(&self, username: &str) -> bool { + self.review_usernames.contains(username) + } } #[derive(Deserialize, Serialize)] pub(crate) struct UserPermissionsResponse { github_ids: HashSet, + github_users: HashSet, } enum TeamSource { @@ -74,18 +85,19 @@ impl TeamApiClient { ) -> anyhow::Result { tracing::info!("Reloading permissions for repository {repo}"); - let review_users: HashSet = self + let (review_users, review_usernames) = self .load_users(repo.name(), PermissionType::Review) .await .map_err(|error| anyhow::anyhow!("Cannot load review users: {error:?}"))?; - let try_users = self - .load_users(repo.name(), PermissionType::Try) - .await - .map_err(|error| anyhow::anyhow!("Cannot load try users: {error:?}"))?; + let (try_users, _try_usernames) = + self.load_users(repo.name(), PermissionType::Try) + .await + .map_err(|error| anyhow::anyhow!("Cannot load try users: {error:?}"))?; Ok(UserPermissions { review_users, + review_usernames, try_users, }) } @@ -96,7 +108,7 @@ impl TeamApiClient { &self, repository_name: &str, permission: PermissionType, - ) -> anyhow::Result> { + ) -> anyhow::Result<(HashSet, HashSet)> { let permission = match permission { PermissionType::Review => "review", PermissionType::Try => "try", @@ -116,7 +128,7 @@ impl TeamApiClient { .map_err(|error| { anyhow::anyhow!("Cannot deserialize users from team API: {error:?}") })?; - Ok(users.github_ids) + Ok((users.github_ids, users.github_users)) } TeamSource::Directory(base_path) => { let path = format!("{base_path}/bors.{permission}.json"); @@ -127,7 +139,7 @@ impl TeamApiClient { serde_json::from_str(&data).map_err(|error| { anyhow::anyhow!("Cannot deserialize users from a file '{path}': {error:?}") })?; - Ok(users.github_ids) + Ok((users.github_ids, users.github_users)) } } } From 86c8f5060290a12f0e955079dd81008d90f0f84a Mon Sep 17 00:00:00 2001 From: reddevilmidzy Date: Tue, 27 Jan 2026 21:45:03 +0900 Subject: [PATCH 4/4] Add test users to permissions mock and update reviewer tests --- src/bors/handlers/review.rs | 35 +++++++++++++++++++++++++++++++---- src/tests/mock/permissions.rs | 3 ++- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index b61266a0..b0206800 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -518,6 +518,7 @@ mod tests { use crate::bors::TRY_BRANCH_NAME; use crate::bors::merge_queue::AUTO_BUILD_CHECK_RUN_NAME; use crate::database::{DelegatedPermission, OctocrabMergeableState, TreeState}; + use crate::permissions::PermissionType; use crate::tests::default_repo_name; use crate::tests::{BorsTester, Commit}; use crate::{ @@ -565,8 +566,16 @@ approved = ["+approved"] #[sqlx::test] async fn approve_on_behalf(pool: sqlx::PgPool) { - run_test(pool, async |ctx: &mut BorsTester| { - let approve_user = "user1"; + let approve_user_id = 200; + let approve_user = "user1"; + let mut gh = GitHub::default(); + gh.add_user(User::new(approve_user_id, approve_user)); + gh.default_repo().lock().permissions.users.insert( + User::new(approve_user_id, approve_user), + vec![PermissionType::Review], + ); + + run_test((pool, gh), async |ctx: &mut BorsTester| { ctx.post_comment(format!(r#"@bors r={approve_user}"#).as_str()) .await?; insta::assert_snapshot!( @@ -586,7 +595,15 @@ approved = ["+approved"] #[sqlx::test] async fn approve_normalize_approver(pool: sqlx::PgPool) { - run_test(pool, async |ctx: &mut BorsTester| { + let mut gh = GitHub::default(); + gh.add_user(User::new(201, "foo")); + gh.default_repo() + .lock() + .permissions + .users + .insert(User::new(201, "foo"), vec![PermissionType::Review]); + + run_test((pool, gh), async |ctx: &mut BorsTester| { ctx.post_comment("@bors r=@foo").await?; insta::assert_snapshot!( ctx.get_next_comment_text(()).await?, @@ -605,7 +622,17 @@ approved = ["+approved"] #[sqlx::test] async fn approve_normalize_approver_multiple(pool: sqlx::PgPool) { - run_test(pool, async |ctx: &mut BorsTester| { + let mut gh = GitHub::default(); + for (id, name) in [(202, "foo"), (203, "bar"), (204, "baz")] { + gh.add_user(User::new(id, name)); + gh.default_repo() + .lock() + .permissions + .users + .insert(User::new(id, name), vec![PermissionType::Review]); + } + + run_test((pool, gh), async |ctx: &mut BorsTester| { ctx.post_comment("@bors r=@foo,bar,@baz").await?; insta::assert_snapshot!( ctx.get_next_comment_text(()).await?, diff --git a/src/tests/mock/permissions.rs b/src/tests/mock/permissions.rs index 642814a9..55140258 100644 --- a/src/tests/mock/permissions.rs +++ b/src/tests/mock/permissions.rs @@ -23,7 +23,8 @@ impl TeamApiMockServer { users.sort_by_key(|p| p.0.github_id); users.retain(|p| p.1.contains(&kind)); let permissions = json!({ - "github_ids": users.into_iter().map(|(user, _)| user.github_id).collect::>() + "github_ids": users.iter().map(|(user, _)| user.github_id).collect::>(), + "github_users": users.iter().map(|(user, _)| user.name.clone()).collect::>() }); Mock::given(method("GET"))