Merge pull request #366 from Kobzol/pr-approval-sanity-checks

Do not allow approving WIP PRs

Author: Kobzol

src/bors/comment.rs

@@ -218,6 +218,15 @@ pub fn approve_non_open_pr_comment() -> Comment {
     Comment::new("Only open, non-draft PRs can be approved".to_string())
 }
 
+pub fn approve_wip_title(keyword: &str) -> Comment {
+    Comment::new(format!(
+        r":clipboard: Looks like this PR is still in progress, ignoring approval.
+
+Hint: Remove **{keyword}** from this PR's title when it is ready for review.
+"
+    ))
+}
+
 pub fn delegate_try_builds_comment(delegatee: &str, bot_prefix: &CommandPrefix) -> Comment {
     Comment::new(format!(
         r":v: @{delegatee}, you can now perform try builds on this pull request!

src/bors/handlers/review.rs

@@ -5,7 +5,7 @@ use crate::bors::RepositoryState;
 use crate::bors::command::RollupMode;
 use crate::bors::command::{Approver, CommandPrefix};
 use crate::bors::comment::{
-    approve_non_open_pr_comment, delegate_comment, delegate_try_builds_comment,
+    approve_non_open_pr_comment, approve_wip_title, delegate_comment, delegate_try_builds_comment,
 };
 use crate::bors::handlers::has_permission;
 use crate::bors::handlers::labels::handle_label_trigger;
@@ -38,10 +38,10 @@ pub(super) async fn command_approve(
         return Ok(());
     };
 
-    if !matches!(pr.github.status, PullRequestStatus::Open) {
+    if let Some(error_comment) = check_pr_approval_validity(pr).await? {
         repo_state
             .client
-            .post_comment(pr.number(), approve_non_open_pr_comment())
+            .post_comment(pr.number(), error_comment)
             .await?;
         return Ok(());
     }
@@ -62,6 +62,28 @@ pub(super) async fn command_approve(
     notify_of_approval(&repo_state, pr, approver.as_str()).await
 }
 
+/// 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]"];
+
+/// Check that the given PR can be approved in its current state.
+/// Returns `Ok(Some(comment))` if it **cannot** be approved; the comment should be sent to the
+/// pull request.
+async fn check_pr_approval_validity(pr: &PullRequestData) -> anyhow::Result<Option<Comment>> {
+    // Check PR status
+    if !matches!(pr.github.status, PullRequestStatus::Open) {
+        return Ok(Some(approve_non_open_pr_comment()));
+    }
+
+    // Check WIP title
+    let title = pr.github.title.to_lowercase();
+    if let Some(wip_kw) = WIP_KEYWORDS.iter().find(|kw| title.contains(*kw)) {
+        return Ok(Some(approve_wip_title(wip_kw)));
+    }
+
+    Ok(None)
+}
+
 /// Unapprove a pull request.
 /// Pull request's author can also unapprove the pull request.
 pub(super) async fn command_unapprove(
@@ -288,6 +310,7 @@ async fn notify_of_delegation(
 #[cfg(test)]
 mod tests {
     use crate::database::{DelegatedPermission, TreeState};
+    use crate::tests::BorsTester;
     use crate::{
         bors::{
             RollupMode,
@@ -1113,4 +1136,24 @@ mod tests {
         })
             .await;
     }
+
+    #[sqlx::test]
+    async fn approve_wip_pr(pool: sqlx::PgPool) {
+        run_test(pool, async |tester: &mut BorsTester| {
+            tester
+                .edit_pr(default_repo_name(), default_pr_number(), |pr| {
+                    pr.title = "[do not merge] CI experiments".to_string();
+                })
+                .await?;
+            tester.post_comment("@bors r+").await?;
+            insta::assert_snapshot!(tester.get_comment().await?, @r"
+            :clipboard: Looks like this PR is still in progress, ignoring approval.
+
+            Hint: Remove **[do not merge]** from this PR's title when it is ready for review.
+            ");
+            tester.default_pr().await.expect_unapproved();
+            Ok(())
+        })
+        .await;
+    }
 }

src/bors/handlers/trybuild.rs

@@ -562,7 +562,7 @@ It fixes so many issues, sir."
 
             insta::assert_snapshot!(tester.get_branch_commit_message(&tester.try_branch()), @r"
             Auto merge of rust-lang/borstest#1 - pr-1, r=<try>
-            PR #1
+            Title of PR 1
             ");
             Ok(())
         })
@@ -591,7 +591,7 @@ try-job: Bar
 
             insta::assert_snapshot!(tester.get_branch_commit_message(&tester.try_branch()), @r"
             Auto merge of rust-lang/borstest#1 - pr-1, r=<try>
-            PR #1
+            Title of PR 1
 
             try-job: Foo
             try-job: Bar
@@ -620,7 +620,7 @@ try-job: Bar
 
             insta::assert_snapshot!(tester.get_branch_commit_message(&tester.try_branch()), @r"
             Auto merge of rust-lang/borstest#1 - pr-1, r=<try>
-            PR #1
+            Title of PR 1
 
 
             try-job: Baz

src/tests/mocks/pull_request.rs

@@ -214,12 +214,13 @@ impl From<PullRequest> for GitHubPullRequest {
             closed_at,
             assignees,
             description,
+            title,
         } = pr;
         GitHubPullRequest {
             user: author.clone().into(),
             url: "https://test.com".to_string(),
             id: number.0 + 1000,
-            title: format!("PR #{number}"),
+            title,
             body: description,
             mergeable_state,
             draft: status == PullRequestStatus::Draft,

src/tests/mocks/repository.rs

@@ -55,6 +55,7 @@ pub struct PullRequest {
     pub closed_at: Option<DateTime<Utc>>,
     pub assignees: Vec<User>,
     pub description: String,
+    pub title: String,
 }
 
 impl PullRequest {
@@ -78,6 +79,7 @@ impl PullRequest {
             closed_at: None,
             assignees: Vec::new(),
             description: format!("Description of PR {number}"),
+            title: format!("Title of PR {number}"),
         }
     }
 }