From 45efb003d61ad7289a09efbfd0546301b5ac81d7 Mon Sep 17 00:00:00 2001 From: Finn Evers Date: Wed, 8 Apr 2026 11:41:22 +0200 Subject: [PATCH] compliance: Check repository status for actor (#53343) Also includes two other minor fixes for stdout info prints Release Notes: - N/A --- tooling/compliance/src/checks.rs | 18 ++++-- tooling/compliance/src/git.rs | 2 +- tooling/compliance/src/github.rs | 82 ++++++++++++++++----------- tooling/compliance/src/report.rs | 4 ++ tooling/xtask/src/tasks/compliance.rs | 10 ++-- 5 files changed, 70 insertions(+), 46 deletions(-) diff --git a/tooling/compliance/src/checks.rs b/tooling/compliance/src/checks.rs index a0623fbbbc179edf9f5b6d777b3116ff498f0265..3c7712593492bf1239e8086099aaef54fad3c275 100644 --- a/tooling/compliance/src/checks.rs +++ b/tooling/compliance/src/checks.rs @@ -146,7 +146,7 @@ impl<'a> Reporter<'a> { if commit.co_authors().is_some() && let Some(commit_authors) = self .github_client - .get_commit_authors([commit.sha()]) + .get_commit_authors(&[commit.sha()]) .await? .get(commit.sha()) .and_then(|authors| authors.co_authors()) @@ -156,7 +156,7 @@ impl<'a> Reporter<'a> { if let Some(github_login) = co_author.user() && self .github_client - .check_org_membership(github_login) + .actor_has_repository_write_permission(github_login) .await? { org_co_authors.push(co_author.clone()); @@ -180,7 +180,7 @@ impl<'a> Reporter<'a> { if let Some(user) = pull_request.user && self .github_client - .check_org_membership(&GithubLogin::new(user.login)) + .actor_has_repository_write_permission(&GithubLogin::new(user.login)) .await? .not() { @@ -219,7 +219,9 @@ impl<'a> Reporter<'a> { .is_some_and(|state| state == ReviewState::Approved) && self .github_client - .check_org_membership(&GithubLogin::new(github_login.login.clone())) + .actor_has_repository_write_permission(&GithubLogin::new( + github_login.login.clone(), + )) .await? { org_approving_reviews.push(review); @@ -258,7 +260,9 @@ impl<'a> Reporter<'a> { }) && self .github_client - .check_org_membership(&GithubLogin::new(comment.user.login.clone())) + .actor_has_repository_write_permission(&GithubLogin::new( + comment.user.login.clone(), + )) .await? { org_approving_comments.push(comment); @@ -356,6 +360,10 @@ mod tests { .any(|member| member == login.as_str())) } + async fn check_repo_write_permission(&self, _login: &GithubLogin) -> anyhow::Result { + Ok(false) + } + async fn ensure_pull_request_has_label( &self, _label: &str, diff --git a/tooling/compliance/src/git.rs b/tooling/compliance/src/git.rs index fa2cb725712de82526d4ce717c2ec3dc97d22885..1ec5f6866305f7bef89ae56ff3631d5e2998b048 100644 --- a/tooling/compliance/src/git.rs +++ b/tooling/compliance/src/git.rs @@ -221,7 +221,7 @@ impl CommitList { self.0 .first() .zip(self.0.last()) - .map(|(first, last)| format!("{}..{}", first.sha().0, last.sha().0)) + .map(|(first, last)| format!("{}..{}", last.sha().0, first.sha().0)) } } diff --git a/tooling/compliance/src/github.rs b/tooling/compliance/src/github.rs index ebd2f2c75f5d0083632a8f70e3ea9dd2680d4eb5..d312dcb1ac3cc27f91a9a93baf1e61a3a8c552c4 100644 --- a/tooling/compliance/src/github.rs +++ b/tooling/compliance/src/github.rs @@ -44,7 +44,7 @@ pub struct GithubLogin { } impl GithubLogin { - pub(crate) fn new(login: String) -> Self { + pub fn new(login: String) -> Self { Self { login } } } @@ -114,9 +114,18 @@ pub trait GitHubApiClient { async fn get_pull_request_comments(&self, pr_number: u64) -> Result>; async fn get_commit_authors(&self, commit_shas: &[&CommitSha]) -> Result; async fn check_org_membership(&self, login: &GithubLogin) -> Result; + async fn check_repo_write_permission(&self, login: &GithubLogin) -> Result; + async fn actor_has_repository_write_permission( + &self, + login: &GithubLogin, + ) -> anyhow::Result { + Ok(self.check_org_membership(login).await? + || self.check_repo_write_permission(login).await?) + } async fn ensure_pull_request_has_label(&self, label: &str, pr_number: u64) -> Result<()>; } +#[derive(Deref)] pub struct GitHubClient { api: Rc, } @@ -131,39 +140,6 @@ impl GitHubClient { let client = OctocrabClient::new(app_id, app_private_key).await?; Ok(Self::new(Rc::new(client))) } - - pub async fn get_pull_request(&self, pr_number: u64) -> Result { - self.api.get_pull_request(pr_number).await - } - - pub async fn get_pull_request_reviews(&self, pr_number: u64) -> Result> { - self.api.get_pull_request_reviews(pr_number).await - } - - pub async fn get_pull_request_comments( - &self, - pr_number: u64, - ) -> Result> { - self.api.get_pull_request_comments(pr_number).await - } - - pub async fn get_commit_authors<'a>( - &self, - commit_shas: impl IntoIterator, - ) -> Result { - let shas: Vec<&CommitSha> = commit_shas.into_iter().collect(); - self.api.get_commit_authors(&shas).await - } - - pub async fn check_org_membership(&self, login: &GithubLogin) -> Result { - self.api.check_org_membership(login).await - } - - pub async fn add_label_to_pull_request(&self, label: &str, pr_number: u64) -> Result<()> { - self.api - .ensure_pull_request_has_label(label, pr_number) - .await - } } #[cfg(feature = "octo-client")] @@ -395,6 +371,44 @@ mod octo_client { .any(|member| member.login == login.as_str())) } + async fn check_repo_write_permission(&self, login: &GithubLogin) -> Result { + // TODO: octocrab fails to deserialize the permission response and + // does not adhere to the scheme laid out at + // https://docs.github.com/en/rest/collaborators/collaborators?apiVersion=2026-03-10#get-repository-permissions-for-a-user + + #[derive(serde::Deserialize)] + #[serde(rename_all = "lowercase")] + enum RepoPermission { + Admin, + Write, + Read, + #[serde(other)] + Other, + } + + #[derive(serde::Deserialize)] + struct RepositoryPermissions { + permission: RepoPermission, + } + + self.client + .get::( + format!( + "/repos/{ORG}/{REPO}/collaborators/{user}/permission", + user = login.as_str() + ), + None::<&()>, + ) + .await + .map(|response| { + matches!( + response.permission, + RepoPermission::Write | RepoPermission::Admin + ) + }) + .map_err(Into::into) + } + async fn ensure_pull_request_has_label(&self, label: &str, pr_number: u64) -> Result<()> { if self .get_filtered( diff --git a/tooling/compliance/src/report.rs b/tooling/compliance/src/report.rs index 16df145394726b97382884fbdfdc3164c0029786..21ec8394c231ffd780c38751cd3e28c255648670 100644 --- a/tooling/compliance/src/report.rs +++ b/tooling/compliance/src/report.rs @@ -115,6 +115,10 @@ impl ReportSummary { fn has_errors(&self) -> bool { self.errors > 0 } + + pub fn prs_with_errors(&self) -> usize { + self.pull_requests - self.reviewed + } } #[derive(Clone, Copy, Debug, Display, PartialEq, Eq, PartialOrd, Ord)] diff --git a/tooling/xtask/src/tasks/compliance.rs b/tooling/xtask/src/tasks/compliance.rs index 78cc32b23f3160ae950aaa5e374071dd107ec350..f6a80e06826bdc1f96739f0113b648c0b6296653 100644 --- a/tooling/xtask/src/tasks/compliance.rs +++ b/tooling/xtask/src/tasks/compliance.rs @@ -92,7 +92,7 @@ async fn check_compliance_impl(args: ComplianceArgs) -> Result<()> { println!( "Applying compliance labels to {} pull requests", - summary.pull_requests + summary.prs_with_errors() ); for report in report.errors() { @@ -100,16 +100,14 @@ async fn check_compliance_impl(args: ComplianceArgs) -> Result<()> { println!("Adding review label to PR {}...", pr_number); client - .add_label_to_pull_request(compliance::github::PR_REVIEW_LABEL, pr_number) + .ensure_pull_request_has_label(compliance::github::PR_REVIEW_LABEL, pr_number) .await?; } } - let report_path = args.report_path.with_extension("md"); + report.write_markdown(&args.report_path)?; - report.write_markdown(&report_path)?; - - println!("Wrote compliance report to {}", report_path.display()); + println!("Wrote compliance report to {}", args.report_path.display()); match summary.review_summary() { ReportReviewSummary::MissingReviews => Err(anyhow::anyhow!(