Skip to content

Commit 619e877

Browse files
authored
fix: prevent PR review summary exceeding 65535 len (#370)
resolves #75 This simply focuses on the length of the patch, because all other parts of the PR review summary will fit under 65535 characters without the patch added.
1 parent 7253c7c commit 619e877

2 files changed

Lines changed: 60 additions & 16 deletions

File tree

cpp-linter/src/clang_tools/mod.rs

Lines changed: 57 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,7 @@ impl ReviewComments {
250250
];
251251
for (tool_name, tool_version) in versions {
252252
if let Some(ver) = tool_version {
253+
// If a tool was used, then we know it's version at this point.
253254
body.push_str(format!("### Used {tool_name} v{ver}\n").as_str());
254255
}
255256
}
@@ -286,19 +287,30 @@ impl ReviewComments {
286287
// The `full_patch` includes all suggestions that didn't fit in the diff.
287288
// It can also contain suggestions that were duplicates of previous reviews.
288289
if !self.full_patch.is_empty() {
289-
body.push_str("\n<details><summary>Click here for ");
290+
let current_len = body.len() + USER_OUTREACH.len();
291+
let mut patch_prefix = "\n<details><summary>Click here for ".to_string();
290292
if summary_only {
291-
body.push_str("the full patch of fixes");
293+
patch_prefix.push_str("the full patch of fixes");
292294
} else {
293-
body.push_str("a patch of fixes outside the diff");
295+
patch_prefix.push_str("a patch of fixes outside the diff");
296+
}
297+
patch_prefix.push_str("</summary><p>\n\n```diff\n");
298+
let patch_suffix = "```\n\n</p></details>\n";
299+
300+
if (current_len + patch_prefix.len() + self.full_patch.len() + patch_suffix.len())
301+
> u16::MAX as usize
302+
{
303+
log::warn!(
304+
"The full patch of fixes is too large to include in the review summary."
305+
);
306+
body.push_str(
307+
"\nThe full patch of fixes is too large to include in this summary.\n",
308+
);
309+
} else {
310+
body.push_str(&patch_prefix);
311+
body.push_str(self.full_patch.as_str());
312+
body.push_str(patch_suffix);
294313
}
295-
body.push_str(
296-
format!(
297-
"</summary><p>\n\n```diff\n{}```\n\n</p></details>\n",
298-
self.full_patch
299-
)
300-
.as_str(),
301-
);
302314
} else if total_review_comments == 0 {
303315
// Only congratulate if there was no reused comments
304316
body.push_str("\nNo concerns to report. Great job! :tada:\n");
@@ -377,6 +389,8 @@ mod tests {
377389
test_db_parse(tmp_dir.path()).await.unwrap();
378390
}
379391

392+
const PSEUDO_VERSION: Version = Version::new(15, 0, 0);
393+
380394
#[test]
381395
fn summarize_reused_reviews() {
382396
let comments = vec![ReviewComment {
@@ -385,10 +399,9 @@ mod tests {
385399
comment: "First comment".to_string(),
386400
path: "src/demo.cpp".to_string(),
387401
}];
388-
let pseudo_version = Version::new(15, 0, 0);
389402
let clang_versions = ClangVersions {
390-
format_version: Some(pseudo_version.clone()),
391-
tidy_version: Some(pseudo_version),
403+
format_version: Some(PSEUDO_VERSION.clone()),
404+
tidy_version: Some(PSEUDO_VERSION),
392405
};
393406
let total_review_comments = 2;
394407
let summary_only = false;
@@ -400,4 +413,35 @@ mod tests {
400413
);
401414
assert!(review_summary.contains("suggestions were duplicates of previous reviews"));
402415
}
416+
417+
#[test]
418+
fn summary_len_truncated() {
419+
let comments = vec![ReviewComment {
420+
line_start: Some(1),
421+
line_end: 1,
422+
comment: "First comment".to_string(),
423+
path: "src/demo.cpp".to_string(),
424+
}];
425+
let clang_versions = ClangVersions {
426+
format_version: Some(PSEUDO_VERSION.clone()),
427+
tidy_version: Some(PSEUDO_VERSION),
428+
};
429+
let total_review_comments = 2;
430+
let summary_only = false;
431+
let long_patch = "a".repeat(u16::MAX as usize);
432+
let review_summary = ReviewComments {
433+
full_patch: long_patch,
434+
..Default::default()
435+
}
436+
.summarize(
437+
&clang_versions,
438+
&comments,
439+
total_review_comments,
440+
summary_only,
441+
);
442+
assert!(
443+
review_summary
444+
.contains("The full patch of fixes is too large to include in this summary.")
445+
);
446+
}
403447
}

cpp-linter/src/rest_client.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -156,13 +156,13 @@ impl RestClient {
156156

157157
if feedback_inputs.thread_comments != ThreadComments::Off {
158158
// post thread comment for PR or push event
159-
if comment.as_ref().is_none_or(|c| c.len() > 65535) {
159+
if comment.as_ref().is_none_or(|c| c.len() > u16::MAX as usize) {
160160
comment = Some(Self::make_comment(
161161
files,
162162
format_checks_failed,
163163
tidy_checks_failed,
164164
&clang_versions,
165-
Some(65535),
165+
Some(u16::MAX as u64),
166166
)?);
167167
}
168168
let options = ThreadCommentOptions {
@@ -514,7 +514,7 @@ mod test {
514514
}
515515
let mut files = vec![];
516516
if !is_lgtm {
517-
for _i in 0..65535 {
517+
for _i in 0..u16::MAX {
518518
let filename = String::from("tests/demo/demo.cpp");
519519
let mut file = FileObj::new(PathBuf::from(&filename));
520520
let notes = vec![TidyNotification {

0 commit comments

Comments
 (0)