From c4661cf85d084f7bb33ccbca2717835b0e94ee8c Mon Sep 17 00:00:00 2001 From: Finn Evers Date: Wed, 8 Apr 2026 22:11:08 +0200 Subject: [PATCH] compliance: Also check for approval pattern in pull request reviews (#53431) Release Notes: - N/A --- tooling/compliance/src/checks.rs | 85 +++++++++++++++++++++++++++++--- tooling/compliance/src/github.rs | 11 +++++ tooling/compliance/src/report.rs | 1 + 3 files changed, 90 insertions(+), 7 deletions(-) diff --git a/tooling/compliance/src/checks.rs b/tooling/compliance/src/checks.rs index 3c7712593492bf1239e8086099aaef54fad3c275..e9d86e7aa3f1924c8b1e619f5890bc8673d3a66f 100644 --- a/tooling/compliance/src/checks.rs +++ b/tooling/compliance/src/checks.rs @@ -117,7 +117,10 @@ impl<'a> Reporter<'a> { let pull_request = self.github_client.get_pull_request(pr_number).await?; - if let Some(approval) = self.check_pull_request_approved(&pull_request).await? { + if let Some(approval) = self + .check_approving_pull_request_review(&pull_request) + .await? + { return Ok(approval); } @@ -197,7 +200,7 @@ impl<'a> Reporter<'a> { } } - async fn check_pull_request_approved( + async fn check_approving_pull_request_review( &self, pull_request: &PullRequestData, ) -> Result, ReviewFailure> { @@ -214,9 +217,13 @@ impl<'a> Reporter<'a> { .user .as_ref() .is_none_or(|pr_user| pr_user.login != github_login.login) - && review + && (review .state .is_some_and(|state| state == ReviewState::Approved) + || review + .body + .as_deref() + .is_some_and(Self::contains_approving_pattern)) && self .github_client .actor_has_repository_write_permission(&GithubLogin::new( @@ -254,10 +261,10 @@ impl<'a> Reporter<'a> { .user .as_ref() .is_some_and(|pr_author| pr_author.login != comment.user.login) - && comment.body.as_ref().is_some_and(|body| { - body.contains(ZED_ZIPPY_COMMENT_APPROVAL_PATTERN) - || body.contains(ZED_ZIPPY_GROUP_APPROVAL) - }) + && comment + .body + .as_deref() + .is_some_and(Self::contains_approving_pattern) && self .github_client .actor_has_repository_write_permission(&GithubLogin::new( @@ -278,6 +285,10 @@ impl<'a> Reporter<'a> { } } + fn contains_approving_pattern(body: &str) -> bool { + body.contains(ZED_ZIPPY_COMMENT_APPROVAL_PATTERN) || body.contains(ZED_ZIPPY_GROUP_APPROVAL) + } + pub async fn generate_report(mut self) -> anyhow::Result { let mut report = Report::new(); @@ -397,6 +408,7 @@ mod tests { login: login.to_owned(), }), state: Some(state), + body: None, } } @@ -652,4 +664,63 @@ mod tests { let result = TestScenario::single_commit().run_scenario().await; assert!(matches!(result, Err(ReviewFailure::Unreviewed))); } + + #[tokio::test] + async fn review_with_zippy_approval_body_is_accepted() { + let result = TestScenario::single_commit() + .with_reviews(vec![ + review("bob", ReviewState::Other).with_body("@zed-zippy approve"), + ]) + .with_org_members(vec!["bob"]) + .run_scenario() + .await; + assert!(matches!(result, Ok(ReviewSuccess::PullRequestReviewed(_)))); + } + + #[tokio::test] + async fn review_with_group_approval_body_is_accepted() { + let result = TestScenario::single_commit() + .with_reviews(vec![ + review("bob", ReviewState::Other).with_body("@zed-industries/approved"), + ]) + .with_org_members(vec!["bob"]) + .run_scenario() + .await; + assert!(matches!(result, Ok(ReviewSuccess::PullRequestReviewed(_)))); + } + + #[tokio::test] + async fn review_with_non_approving_body_is_not_accepted() { + let result = TestScenario::single_commit() + .with_reviews(vec![ + review("bob", ReviewState::Other).with_body("looks good to me"), + ]) + .with_org_members(vec!["bob"]) + .run_scenario() + .await; + assert!(matches!(result, Err(ReviewFailure::Unreviewed))); + } + + #[tokio::test] + async fn review_with_approving_body_from_external_user_is_not_accepted() { + let result = TestScenario::single_commit() + .with_reviews(vec![ + review("bob", ReviewState::Other).with_body("@zed-zippy approve"), + ]) + .run_scenario() + .await; + assert!(matches!(result, Err(ReviewFailure::Unreviewed))); + } + + #[tokio::test] + async fn review_with_approving_body_from_pr_author_is_rejected() { + let result = TestScenario::single_commit() + .with_reviews(vec![ + review("alice", ReviewState::Other).with_body("@zed-zippy approve"), + ]) + .with_org_members(vec!["alice"]) + .run_scenario() + .await; + assert!(matches!(result, Err(ReviewFailure::Unreviewed))); + } } diff --git a/tooling/compliance/src/github.rs b/tooling/compliance/src/github.rs index d312dcb1ac3cc27f91a9a93baf1e61a3a8c552c4..96a786ccb2a1a16f31688bf3a87f1d862c195838 100644 --- a/tooling/compliance/src/github.rs +++ b/tooling/compliance/src/github.rs @@ -30,6 +30,16 @@ pub enum ReviewState { pub struct PullRequestReview { pub user: Option, pub state: Option, + pub body: Option, +} + +impl PullRequestReview { + pub fn with_body(self, body: impl ToString) -> Self { + Self { + body: Some(body.to_string()), + ..self + } + } } #[derive(Debug, Clone)] @@ -294,6 +304,7 @@ mod octo_client { OctocrabReviewState::Approved => ReviewState::Approved, _ => ReviewState::Other, }), + body: review.body, }) .collect()) } diff --git a/tooling/compliance/src/report.rs b/tooling/compliance/src/report.rs index 21ec8394c231ffd780c38751cd3e28c255648670..36e648942327815012680b9661b6bbeb77389ae1 100644 --- a/tooling/compliance/src/report.rs +++ b/tooling/compliance/src/report.rs @@ -334,6 +334,7 @@ mod tests { login: "reviewer".to_owned(), }), state: Some(ReviewState::Approved), + body: None, }]) }