Skip to content

feat: skip posting thread or review comments on PRs from forks #557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
2bndy5 merged 2 commits into from
Nov 9, 2026
Merged

feat: skip posting thread or review comments on PRs from forks #557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b29631e
feat: skip posting thread or review comments on PRs from forks
2bndy5 Nov 9, 2026
3fd5e35
test: simulate PR from fork
2bndy5 Nov 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 47 additions & 37 deletions cpp-linter/src/rest_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
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 {
Expand All @@ -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(
Expand Down
58 changes: 55 additions & 3 deletions cpp-linter/tests/comments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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" {
Expand Down Expand Up @@ -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())
Expand All @@ -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())
Expand All @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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;
}
33 changes: 30 additions & 3 deletions cpp-linter/tests/reviews.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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()
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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(
Expand All @@ -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())
Expand Down Expand Up @@ -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()
);
}
Loading