diff --git a/tooling/compliance/src/checks.rs b/tooling/compliance/src/checks.rs index 6d5116b4a8c..570c87b6554 100644 --- a/tooling/compliance/src/checks.rs +++ b/tooling/compliance/src/checks.rs @@ -5,8 +5,8 @@ use itertools::Itertools as _; use crate::{ git::{CommitDetails, CommitList, ZED_ZIPPY_LOGIN}, github::{ - CommitAuthor, GithubApiClient, GithubLogin, PullRequestComment, PullRequestData, - PullRequestReview, Repository, ReviewState, + Approvable, CommitAuthor, GithubApiClient, GithubLogin, PullRequestComment, + PullRequestData, PullRequestReview, Repository, ReviewState, }, report::Report, }; @@ -297,87 +297,62 @@ impl Reporter { &self, pull_request: &PullRequestData, ) -> Result, ReviewFailure> { - let pr_reviews = self + let reviews = self .github_client .get_pull_request_reviews(&Repository::ZED, pull_request.number) .await?; - if !pr_reviews.is_empty() { - let mut org_approving_reviews = Vec::new(); - for review in pr_reviews { - if let Some(github_login) = review.user.as_ref() - && pull_request - .user - .as_ref() - .is_none_or(|pr_user| pr_user.login != github_login.login) - && (review - .state - .is_some_and(|state| state == ReviewState::Approved) - || review - .body - .as_deref() - .is_some_and(Self::contains_approving_pattern)) - && self - .github_client - .check_repo_write_permission( - &Repository::ZED, - &GithubLogin::new(github_login.login.clone()), - ) - .await? - { - org_approving_reviews.push(review); - } - } + let qualifying_reviews = reviews + .into_iter() + .filter(|review| Self::is_qualifying_approval(review, pull_request)) + .collect_vec(); - Ok(org_approving_reviews - .is_empty() - .not() - .then_some(ReviewSuccess::PullRequestReviewed(org_approving_reviews))) - } else { - Ok(None) - } + Ok(qualifying_reviews + .is_empty() + .not() + .then_some(ReviewSuccess::PullRequestReviewed(qualifying_reviews))) } async fn check_approving_pull_request_comment( &self, pull_request: &PullRequestData, ) -> Result, ReviewFailure> { - let other_comments = self + let comments = self .github_client .get_pull_request_comments(&Repository::ZED, pull_request.number) .await?; - if !other_comments.is_empty() { - let mut org_approving_comments = Vec::new(); + let qualifying_comments = comments + .into_iter() + .filter(|comment| Self::is_qualifying_approval(comment, pull_request)) + .collect_vec(); - for comment in other_comments { - if pull_request - .user - .as_ref() - .is_some_and(|pr_author| pr_author.login != comment.user.login) - && comment - .body - .as_deref() - .is_some_and(Self::contains_approving_pattern) - && self - .github_client - .check_repo_write_permission( - &Repository::ZED, - &GithubLogin::new(comment.user.login.clone()), - ) - .await? - { - org_approving_comments.push(comment); - } - } + Ok(qualifying_comments + .is_empty() + .not() + .then_some(ReviewSuccess::ApprovingComment(qualifying_comments))) + } - Ok(org_approving_comments - .is_empty() - .not() - .then_some(ReviewSuccess::ApprovingComment(org_approving_comments))) - } else { - Ok(None) - } + pub fn is_qualifying_approval(item: &impl Approvable, pull_request: &PullRequestData) -> bool { + let Some(author_login) = item.author_login() else { + return false; + }; + + let distinct_actor = pull_request + .user + .as_ref() + .is_none_or(|pr_user| pr_user.login != author_login); + + let approving_pattern = item + .review_state() + .is_some_and(|state| state == ReviewState::Approved) + || item.body().is_some_and(Self::contains_approving_pattern); + + let actor_is_authorized = item + .author_association() + .is_some_and(|association| association.has_write_access()); + + distinct_actor && approving_pattern && actor_is_authorized } fn contains_approving_pattern(body: &str) -> bool { @@ -418,8 +393,8 @@ mod tests { use crate::git::{CommitDetails, CommitList, CommitSha, ZED_ZIPPY_EMAIL, ZED_ZIPPY_LOGIN}; use crate::github::{ - CommitMetadataBySha, GithubApiClient, GithubLogin, GithubUser, PullRequestComment, - PullRequestData, PullRequestReview, Repository, ReviewState, + AuthorAssociation, CommitMetadataBySha, GithubApiClient, GithubLogin, GithubUser, + PullRequestComment, PullRequestData, PullRequestReview, Repository, ReviewState, }; use super::{Reporter, ReviewFailure, ReviewSuccess, VersionBumpFailure}; @@ -505,22 +480,32 @@ mod tests { .expect("should have one commit") } - fn review(login: &str, state: ReviewState) -> PullRequestReview { + fn review( + login: &str, + state: ReviewState, + author_association: AuthorAssociation, + ) -> PullRequestReview { PullRequestReview { user: Some(GithubUser { login: login.to_owned(), }), state: Some(state), body: None, + author_association: Some(author_association), } } - fn comment(login: &str, body: &str) -> PullRequestComment { + fn comment( + login: &str, + body: &str, + author_association: AuthorAssociation, + ) -> PullRequestComment { PullRequestComment { user: GithubUser { login: login.to_owned(), }, body: Some(body.to_owned()), + author_association: Some(author_association), } } @@ -665,8 +650,11 @@ mod tests { #[tokio::test] async fn approved_review_by_org_member_succeeds() { let result = TestScenario::single_commit() - .with_reviews(vec![review("bob", ReviewState::Approved)]) - .with_org_members(vec!["bob"]) + .with_reviews(vec![review( + "bob", + ReviewState::Approved, + AuthorAssociation::Member, + )]) .run_scenario() .await; assert!(matches!(result, Ok(ReviewSuccess::PullRequestReviewed(_)))); @@ -675,8 +663,11 @@ mod tests { #[tokio::test] async fn non_approved_review_state_is_not_accepted() { let result = TestScenario::single_commit() - .with_reviews(vec![review("bob", ReviewState::Other)]) - .with_org_members(vec!["bob"]) + .with_reviews(vec![review( + "bob", + ReviewState::Other, + AuthorAssociation::Member, + )]) .run_scenario() .await; assert!(matches!(result, Err(ReviewFailure::Unreviewed))); @@ -685,7 +676,11 @@ mod tests { #[tokio::test] async fn review_by_non_org_member_is_not_accepted() { let result = TestScenario::single_commit() - .with_reviews(vec![review("bob", ReviewState::Approved)]) + .with_reviews(vec![review( + "bob", + ReviewState::Approved, + AuthorAssociation::None, + )]) .run_scenario() .await; assert!(matches!(result, Err(ReviewFailure::Unreviewed))); @@ -694,8 +689,11 @@ mod tests { #[tokio::test] async fn pr_author_own_approval_review_is_rejected() { let result = TestScenario::single_commit() - .with_reviews(vec![review("alice", ReviewState::Approved)]) - .with_org_members(vec!["alice"]) + .with_reviews(vec![review( + "alice", + ReviewState::Approved, + AuthorAssociation::Member, + )]) .run_scenario() .await; assert!(matches!(result, Err(ReviewFailure::Unreviewed))); @@ -704,8 +702,11 @@ mod tests { #[tokio::test] async fn pr_author_own_approval_comment_is_rejected() { let result = TestScenario::single_commit() - .with_comments(vec![comment("alice", "@zed-zippy approve")]) - .with_org_members(vec!["alice"]) + .with_comments(vec![comment( + "alice", + "@zed-zippy approve", + AuthorAssociation::Member, + )]) .run_scenario() .await; assert!(matches!(result, Err(ReviewFailure::Unreviewed))); @@ -714,8 +715,11 @@ mod tests { #[tokio::test] async fn approval_comment_by_org_member_succeeds() { let result = TestScenario::single_commit() - .with_comments(vec![comment("bob", "@zed-zippy approve")]) - .with_org_members(vec!["bob"]) + .with_comments(vec![comment( + "bob", + "@zed-zippy approve", + AuthorAssociation::Member, + )]) .run_scenario() .await; assert!(matches!(result, Ok(ReviewSuccess::ApprovingComment(_)))); @@ -724,8 +728,11 @@ mod tests { #[tokio::test] async fn group_approval_comment_by_org_member_succeeds() { let result = TestScenario::single_commit() - .with_comments(vec![comment("bob", "@zed-industries/approved")]) - .with_org_members(vec!["bob"]) + .with_comments(vec![comment( + "bob", + "@zed-industries/approved", + AuthorAssociation::Member, + )]) .run_scenario() .await; assert!(matches!(result, Ok(ReviewSuccess::ApprovingComment(_)))); @@ -734,8 +741,11 @@ mod tests { #[tokio::test] async fn comment_without_approval_pattern_is_not_accepted() { let result = TestScenario::single_commit() - .with_comments(vec![comment("bob", "looks good")]) - .with_org_members(vec!["bob"]) + .with_comments(vec![comment( + "bob", + "looks good", + AuthorAssociation::Member, + )]) .run_scenario() .await; assert!(matches!(result, Err(ReviewFailure::Unreviewed))); @@ -759,9 +769,16 @@ mod tests { #[tokio::test] async fn pr_review_takes_precedence_over_comment() { let result = TestScenario::single_commit() - .with_reviews(vec![review("bob", ReviewState::Approved)]) - .with_comments(vec![comment("charlie", "@zed-zippy approve")]) - .with_org_members(vec!["bob", "charlie"]) + .with_reviews(vec![review( + "bob", + ReviewState::Approved, + AuthorAssociation::Member, + )]) + .with_comments(vec![comment( + "charlie", + "@zed-zippy approve", + AuthorAssociation::Member, + )]) .run_scenario() .await; assert!(matches!(result, Ok(ReviewSuccess::PullRequestReviewed(_)))); @@ -770,7 +787,11 @@ mod tests { #[tokio::test] async fn comment_takes_precedence_over_co_author() { let result = TestScenario::single_commit() - .with_comments(vec![comment("bob", "@zed-zippy approve")]) + .with_comments(vec![comment( + "bob", + "@zed-zippy approve", + AuthorAssociation::Member, + )]) .with_commit_metadata_json(serde_json::json!({ "abc12345abc12345": { "author": alice_author(), @@ -784,7 +805,6 @@ mod tests { "Fix thing (#1234)", "Co-authored-by: Charlie ", )) - .with_org_members(vec!["bob", "charlie"]) .run_scenario() .await; assert!(matches!(result, Ok(ReviewSuccess::ApprovingComment(_)))); @@ -822,9 +842,9 @@ mod tests { 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"), + review("bob", ReviewState::Other, AuthorAssociation::Member) + .with_body("@zed-zippy approve"), ]) - .with_org_members(vec!["bob"]) .run_scenario() .await; assert!(matches!(result, Ok(ReviewSuccess::PullRequestReviewed(_)))); @@ -834,9 +854,9 @@ mod tests { 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"), + review("bob", ReviewState::Other, AuthorAssociation::Member) + .with_body("@zed-industries/approved"), ]) - .with_org_members(vec!["bob"]) .run_scenario() .await; assert!(matches!(result, Ok(ReviewSuccess::PullRequestReviewed(_)))); @@ -846,9 +866,9 @@ mod tests { 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"), + review("bob", ReviewState::Other, AuthorAssociation::Member) + .with_body("looks good to me"), ]) - .with_org_members(vec!["bob"]) .run_scenario() .await; assert!(matches!(result, Err(ReviewFailure::Unreviewed))); @@ -858,7 +878,8 @@ mod tests { 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"), + review("bob", ReviewState::Other, AuthorAssociation::None) + .with_body("@zed-zippy approve"), ]) .run_scenario() .await; @@ -869,9 +890,9 @@ mod tests { 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"), + review("alice", ReviewState::Other, AuthorAssociation::Member) + .with_body("@zed-zippy approve"), ]) - .with_org_members(vec!["alice"]) .run_scenario() .await; assert!(matches!(result, Err(ReviewFailure::Unreviewed))); @@ -1052,8 +1073,11 @@ mod tests { "Some change (#1234)", "", )) - .with_reviews(vec![review("bob", ReviewState::Approved)]) - .with_org_members(vec!["bob"]) + .with_reviews(vec![review( + "bob", + ReviewState::Approved, + AuthorAssociation::Member, + )]) .run_scenario() .await; assert!(matches!(result, Ok(ReviewSuccess::PullRequestReviewed(_)))); diff --git a/tooling/compliance/src/github.rs b/tooling/compliance/src/github.rs index d6ebcc227d7..9a2333078f9 100644 --- a/tooling/compliance/src/github.rs +++ b/tooling/compliance/src/github.rs @@ -27,11 +27,37 @@ pub enum ReviewState { Other, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum AuthorAssociation { + Owner, + Member, + Collaborator, + Contributor, + FirstTimeContributor, + FirstTimer, + Mannequin, + None, +} + +impl AuthorAssociation { + pub fn has_write_access(&self) -> bool { + matches!(self, Self::Owner | Self::Member | Self::Collaborator) + } +} + +pub trait Approvable { + fn author_login(&self) -> Option<&str>; + fn review_state(&self) -> Option; + fn body(&self) -> Option<&str>; + fn author_association(&self) -> Option; +} + #[derive(Debug, Clone)] pub struct PullRequestReview { pub user: Option, pub state: Option, pub body: Option, + pub author_association: Option, } impl PullRequestReview { @@ -43,10 +69,47 @@ impl PullRequestReview { } } +impl Approvable for PullRequestReview { + fn author_login(&self) -> Option<&str> { + self.user.as_ref().map(|user| user.login.as_str()) + } + + fn review_state(&self) -> Option { + self.state + } + + fn body(&self) -> Option<&str> { + self.body.as_deref() + } + + fn author_association(&self) -> Option { + self.author_association + } +} + #[derive(Debug, Clone)] pub struct PullRequestComment { pub user: GithubUser, pub body: Option, + pub author_association: Option, +} + +impl Approvable for PullRequestComment { + fn author_login(&self) -> Option<&str> { + Some(&self.user.login) + } + + fn review_state(&self) -> Option { + None + } + + fn body(&self) -> Option<&str> { + self.body.as_deref() + } + + fn author_association(&self) -> Option { + self.author_association + } } #[derive(Debug, Deserialize, Clone, Deref, PartialEq, Eq)] @@ -351,7 +414,11 @@ mod octo_client { use futures::TryStreamExt as _; use jsonwebtoken::EncodingKey; use octocrab::{ - Octocrab, Page, models::pulls::ReviewState as OctocrabReviewState, + Octocrab, Page, + models::{ + AuthorAssociation as OctocrabAuthorAssociation, + pulls::ReviewState as OctocrabReviewState, + }, service::middleware::cache::mem::InMemoryCache, }; use serde::de::DeserializeOwned; @@ -363,10 +430,26 @@ mod octo_client { }; use super::{ - CommitMetadataBySha, GithubApiClient, GithubLogin, GithubUser, PullRequestComment, - PullRequestData, PullRequestReview, ReviewState, + AuthorAssociation, CommitMetadataBySha, GithubApiClient, GithubLogin, GithubUser, + PullRequestComment, PullRequestData, PullRequestReview, ReviewState, }; + fn convert_author_association(association: OctocrabAuthorAssociation) -> AuthorAssociation { + match association { + OctocrabAuthorAssociation::Owner => AuthorAssociation::Owner, + OctocrabAuthorAssociation::Member => AuthorAssociation::Member, + OctocrabAuthorAssociation::Collaborator => AuthorAssociation::Collaborator, + OctocrabAuthorAssociation::Contributor => AuthorAssociation::Contributor, + OctocrabAuthorAssociation::FirstTimeContributor => { + AuthorAssociation::FirstTimeContributor + } + OctocrabAuthorAssociation::FirstTimer => AuthorAssociation::FirstTimer, + OctocrabAuthorAssociation::Mannequin => AuthorAssociation::Mannequin, + OctocrabAuthorAssociation::None => AuthorAssociation::None, + _ => AuthorAssociation::None, + } + } + const PAGE_SIZE: u8 = 100; pub struct OctocrabClient { @@ -481,6 +564,7 @@ mod octo_client { _ => ReviewState::Other, }), body: review.body, + author_association: review.author_association.map(convert_author_association), }) .collect()) } @@ -507,6 +591,7 @@ mod octo_client { login: comment.user.login, }, body: comment.body, + author_association: comment.author_association.map(convert_author_association), }) .collect()) } diff --git a/tooling/compliance/src/report.rs b/tooling/compliance/src/report.rs index 146ab0d3b00..711db20a030 100644 --- a/tooling/compliance/src/report.rs +++ b/tooling/compliance/src/report.rs @@ -321,7 +321,7 @@ mod tests { use crate::{ checks::{ReviewFailure, ReviewSuccess}, git::{CommitDetails, CommitList}, - github::{GithubLogin, GithubUser, PullRequestReview, ReviewState}, + github::{AuthorAssociation, GithubLogin, GithubUser, PullRequestReview, ReviewState}, }; use super::{Report, ReportReviewSummary}; @@ -350,6 +350,7 @@ mod tests { }), state: Some(ReviewState::Approved), body: None, + author_association: Some(AuthorAssociation::Member), }]) }