Warn about unknown reviewers when approving

src/bors/comment.rs

@@ -291,6 +291,7 @@ pub fn approved_comment(
     repo: &GithubRepoName,
     commit_sha: &CommitSha,
     reviewer: &str,
+    unknown_reviewers: Vec<String>,
     tree_state: TreeState,
 ) -> Comment {
     let approve_emoji = if is_holiday_season() {
@@ -305,6 +306,14 @@ It is now in the [queue]({web_url}/queue/{}) for this repository.
 ",
         repo.name()
     );
+    if !unknown_reviewers.is_empty() {
+        writeln!(
+            comment,
+            "\n:warning: The following reviewer(s) could not be found: `{}`",
+            unknown_reviewers.join(", ")
+        )
+        .unwrap();
+    }
     if let TreeState::Closed { priority, source } = tree_state {
         let tree_emoji = if is_holiday_season() {
             "christmas_tree"

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(), Vec::new()),
+        Approver::Specified(approver) => {
+            let normalized = normalize_approvers(approver);
+            let unknown = check_unknown_reviewers(&repo_state, &normalized).await;
+            (normalized.join(","), unknown)
+        }
     };
+
     let approval_info = ApprovalInfo {
         approver: approver.clone(),
         sha: pr.github.head.sha.to_string(),
@@ -83,18 +88,41 @@ 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
 }
 
 /// 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<String> {
     approvers
         .split(',')
-        .map(|approver| approver.trim_start_matches('@'))
-        .collect::<Vec<&str>>()
-        .join(",")
+        .map(|approver| approver.trim_start_matches('@').to_string())
+        .collect::<Vec<String>>()
+}
+
+/// 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: &[String],
+) -> Vec<String> {
+    let permission = repo_state.permissions.load();
+
+    reviewers
+        .iter()
+        .filter(|reviewer| !permission.has_reviewer(reviewer))
+        .cloned()
+        .collect()
 }
 
 /// Keywords that will prevent an approval if they appear in the PR's title.
@@ -424,6 +452,7 @@ async fn notify_of_approval(
     pr: PullRequestData<'_>,
     priority: Option<u32>,
     approver: &str,
+    unknown_reviewers: Vec<String>,
 ) -> anyhow::Result<()> {
     let mut tree_state = ctx
         .db
@@ -451,6 +480,7 @@ async fn notify_of_approval(
                 repo.repository(),
                 &pr.github.head.sha,
                 approver,
+                unknown_reviewers,
                 tree_state,
             ),
             db,
@@ -488,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::{
@@ -535,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!(
@@ -556,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?,
@@ -575,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?,
@@ -592,6 +649,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| {

src/permissions.rs

@@ -24,13 +24,19 @@ impl fmt::Display for PermissionType {
 
 pub struct UserPermissions {
     review_users: HashSet<UserId>,
+    review_usernames: HashSet<String>,
     try_users: HashSet<UserId>,
 }
 
 impl UserPermissions {
-    pub fn new(review_users: HashSet<UserId>, try_users: HashSet<UserId>) -> Self {
+    pub fn new(
+        review_users: HashSet<UserId>,
+        review_usernames: HashSet<String>,
+        try_users: HashSet<UserId>,
+    ) -> 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<UserId>,
+    github_users: HashSet<String>,
 }
 
 enum TeamSource {
@@ -74,18 +85,19 @@ impl TeamApiClient {
     ) -> anyhow::Result<UserPermissions> {
         tracing::info!("Reloading permissions for repository {repo}");
 
-        let review_users: HashSet<UserId> = 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<HashSet<UserId>> {
+    ) -> anyhow::Result<(HashSet<UserId>, HashSet<String>)> {
         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))
             }
         }
     }

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::<Vec<_>>()
+                "github_ids": users.iter().map(|(user, _)| user.github_id).collect::<Vec<_>>(),
+                "github_users": users.iter().map(|(user, _)| user.name.clone()).collect::<Vec<_>>()
             });
 
             Mock::given(method("GET"))