diff --git a/cpp-linter/src/rest_client.rs b/cpp-linter/src/rest_client.rs index a6654c03..941dddaf 100644 --- a/cpp-linter/src/rest_client.rs +++ b/cpp-linter/src/rest_client.rs @@ -268,46 +268,51 @@ impl RestClient { } self.show_patch(&feedback_inputs)?; - let mut options = ReviewOptions { - marker: COMMENT_MARKER.to_string(), - comments: { - let mut comments = vec![]; - for suggestion in &review_comments.comments { - let mut comment = suggestion.as_review_comment(); - comment.path = prefix_feedback_path(&comment.path, &self.repo_root_prefix); - comments.push(comment); - } - comments - }, - ..Default::default() - }; + if self.client.is_pr_same_remote() { + let mut options = ReviewOptions { + marker: COMMENT_MARKER.to_string(), + comments: { + let mut comments = vec![]; + for suggestion in &review_comments.comments { + let mut comment = suggestion.as_review_comment(); + comment.path = + prefix_feedback_path(&comment.path, &self.repo_root_prefix); + comments.push(comment); + } + comments + }, + ..Default::default() + }; - let total_review_comments = options.comments.len() as u32; - self.client.cull_pr_reviews(&mut options).await?; - let has_changes = - review_comments.tool_total > 0 || !review_comments.skipped_fixes.is_empty(); - let is_lgtm = options.comments.is_empty() && !has_changes; - options.action = if feedback_inputs.passive_reviews { - ReviewAction::Comment - } else if is_lgtm { - ReviewAction::Approve - } else { - ReviewAction::RequestChanges - }; - options.summary = review_comments.summarize( - &clang_versions, - &options.comments, - total_review_comments, - summary_only, - ); - if is_lgtm && feedback_inputs.no_lgtm { - log::debug!("Not posting an approved review because `no-lgtm` is true"); - } else if options.comments.is_empty() && total_review_comments > 0 { - log::info!( - "All review comments were already posted in previous reviews; not posting a new review." + let total_review_comments = options.comments.len() as u32; + self.client.cull_pr_reviews(&mut options).await?; + let has_changes = + review_comments.tool_total > 0 || !review_comments.skipped_fixes.is_empty(); + let is_lgtm = options.comments.is_empty() && !has_changes; + options.action = if feedback_inputs.passive_reviews { + ReviewAction::Comment + } else if is_lgtm { + ReviewAction::Approve + } else { + ReviewAction::RequestChanges + }; + options.summary = review_comments.summarize( + &clang_versions, + &options.comments, + total_review_comments, + summary_only, ); + if is_lgtm && feedback_inputs.no_lgtm { + log::debug!("Not posting an approved review because `no-lgtm` is true"); + } else if options.comments.is_empty() && total_review_comments > 0 { + log::info!( + "All review comments were already posted in previous reviews; not posting a new review." + ); + } else { + self.client.post_pr_review(&options).await?; + } } else { - self.client.post_pr_review(&options).await?; + log::warn!("PR is from a different remote, skipping review."); } } else if feedback_inputs.gen_patch { for file in files { @@ -320,6 +325,11 @@ impl RestClient { } if feedback_inputs.thread_comments != ThreadComments::Off { + // do not post thread-comments for on PRs from forks + if self.client.is_pr_event() && !self.client.is_pr_same_remote() { + log::warn!("PR is from a different remote, skipping thread comments."); + return Ok(format_checks_failed + tidy_checks_failed); + } // post thread comment for PR or push event if comment.as_ref().is_none_or(|c| c.len() > u16::MAX as usize) { comment = Some(Self::make_comment( diff --git a/cpp-linter/tests/comments.rs b/cpp-linter/tests/comments.rs index b4885dfc..13ff3412 100644 --- a/cpp-linter/tests/comments.rs +++ b/cpp-linter/tests/comments.rs @@ -45,6 +45,7 @@ struct TestParams { pub thread_comments: ThreadComments, pub no_lgtm: bool, pub force_lgtm: bool, + pub is_fork: bool, pub fail_get_existing_comments: bool, pub fail_dismissal: bool, pub fail_posting: bool, @@ -61,6 +62,7 @@ impl Default for TestParams { thread_comments: ThreadComments::On, no_lgtm: true, force_lgtm: false, + is_fork: false, fail_get_existing_comments: false, fail_dismissal: false, fail_posting: false, @@ -93,6 +95,18 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { "state": "open", "number": PR, "locked": false, + "head": { + "repo": { + "id": if test_params.is_fork { 200 } else { 100 }, + "full_name": if test_params.is_fork { "fork/repo" } else { REPO }, + } + }, + "base": { + "repo": { + "id": 100, + "full_name": REPO, + } + } } }) .to_string(); @@ -154,7 +168,7 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { } mock = mock.create(); mocks.push(mock); - } else { + } else if !test_params.is_fork { let pr_endpoint = format!("/repos/{REPO}/issues/{PR}/comments"); for pg in ["1", "2"] { let link = if pg == "1" { @@ -187,7 +201,10 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { } ); - if !test_params.fail_get_existing_comments && !test_params.bad_existing_comments { + if !test_params.fail_get_existing_comments + && !test_params.bad_existing_comments + && !test_params.is_fork + { mocks.push( server .mock("DELETE", comment_url.as_str()) @@ -212,7 +229,8 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { let lgtm_allowed = !test_params.force_lgtm || !test_params.no_lgtm; - if test_params.thread_comments == ThreadComments::Update && lgtm_allowed { + if test_params.thread_comments == ThreadComments::Update && lgtm_allowed && !test_params.is_fork + { mocks.push( server .mock("PATCH", comment_url.as_str()) @@ -228,6 +246,7 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { if test_params.thread_comments == ThreadComments::On && lgtm_allowed && !test_params.bad_existing_comments + && !test_params.is_fork { mocks.push( server @@ -252,6 +271,18 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { ); } + if test_params.is_fork { + mocks.push( + server + .mock( + "POST", + format!("/repos/{REPO}/issues/{PR}/comments").as_str(), + ) + .expect(0) + .create(), + ); + } + // A relative output path is resolved against the working directory (not the repo root). // `_out_dir` keeps the directory alive until the test ends. let (_out_dir, out_dir_path) = if test_params.relative_out_files { @@ -566,3 +597,24 @@ async fn relative_out_files() { }) .await; } + +#[tokio::test] +async fn new_pr_fork() { + test_comment(&TestParams { + event_t: EventType::PullRequest, + is_fork: true, + ..Default::default() + }) + .await; +} + +#[tokio::test] +async fn update_pr_fork() { + test_comment(&TestParams { + event_t: EventType::PullRequest, + thread_comments: ThreadComments::Update, + is_fork: true, + ..Default::default() + }) + .await; +} diff --git a/cpp-linter/tests/reviews.rs b/cpp-linter/tests/reviews.rs index f5d7f231..60850c0e 100644 --- a/cpp-linter/tests/reviews.rs +++ b/cpp-linter/tests/reviews.rs @@ -36,6 +36,7 @@ struct TestParams { pub summary_only: bool, pub pr_state: String, pub pr_draft: bool, + pub is_fork: bool, pub fail_dismissal: bool, pub fail_get_existing_reviews: bool, pub fail_posting: bool, @@ -56,6 +57,7 @@ impl Default for TestParams { summary_only: false, pr_state: "open".to_string(), pr_draft: false, + is_fork: false, fail_dismissal: false, fail_get_existing_reviews: false, fail_posting: false, @@ -78,6 +80,18 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) -> "state": test_params.pr_state, "number": PR, "locked": false, + "head": { + "repo": { + "id": if test_params.is_fork { 200 } else { 100 }, + "full_name": if test_params.is_fork { "fork/repo" } else { REPO }, + } + }, + "base": { + "repo": { + "id": 100, + "full_name": REPO, + } + } } }) .to_string() @@ -129,7 +143,7 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) -> let reviews_endpoint = format!("/repos/{REPO}/pulls/{PR}/reviews"); - if test_params.pr_state != "closed" && !test_params.bad_pr_info { + if test_params.pr_state != "closed" && !test_params.bad_pr_info && !test_params.is_fork { let mut mock = server .mock("GET", reviews_endpoint.as_str()) .match_header("Accept", "application/vnd.github.raw+json") @@ -167,6 +181,7 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) -> && !test_params.bad_existing_reviews && test_params.pr_state != "closed" && !test_params.bad_pr_info + && !test_params.is_fork { // the previous review is outdated (not reused), so it gets dismissed mocks.push( @@ -186,8 +201,8 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) -> let posted_review = Arc::new(Mutex::new(None)); let lgtm_allowed = !test_params.force_lgtm || !test_params.no_lgtm; - if test_params.previous_review.is_some() { - // all suggestions were reused, so a new review is not posted + if test_params.previous_review.is_some() || test_params.is_fork { + // all suggestions were reused (or is_fork), so a new review is not posted mocks.push( server .mock("POST", reviews_endpoint.as_str()) @@ -520,3 +535,15 @@ async fn all_suggestions_reused() { }) .await; } + +#[tokio::test] +async fn is_fork() { + assert!( + test_review(&TestParams { + is_fork: true, + ..Default::default() + }) + .await + .is_none() + ); +}