Skip to content

Commit 7810388

Browse files
committed
fix: prevent PR review summary exceeding 65535 len
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 7810388

2 files changed

Lines changed: 59 additions & 16 deletions

File tree

cpp-linter/src/clang_tools/mod.rs

Lines changed: 56 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,29 @@ 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+
"\nThe full patch of fixes is too large to include in the review summary.\n"
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());
294312
}
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-
);
302313
} else if total_review_comments == 0 {
303314
// Only congratulate if there was no reused comments
304315
body.push_str("\nNo concerns to report. Great job! :tada:\n");
@@ -377,6 +388,8 @@ mod tests {
377388
test_db_parse(tmp_dir.path()).await.unwrap();
378389
}
379390

391+
const PSEUDO_VERSION: Version = Version::new(15, 0, 0);
392+
380393
#[test]
381394
fn summarize_reused_reviews() {
382395
let comments = vec![ReviewComment {
@@ -385,10 +398,9 @@ mod tests {
385398
comment: "First comment".to_string(),
386399
path: "src/demo.cpp".to_string(),
387400
}];
388-
let pseudo_version = Version::new(15, 0, 0);
389401
let clang_versions = ClangVersions {
390-
format_version: Some(pseudo_version.clone()),
391-
tidy_version: Some(pseudo_version),
402+
format_version: Some(PSEUDO_VERSION.clone()),
403+
tidy_version: Some(PSEUDO_VERSION),
392404
};
393405
let total_review_comments = 2;
394406
let summary_only = false;
@@ -400,4 +412,35 @@ mod tests {
400412
);
401413
assert!(review_summary.contains("suggestions were duplicates of previous reviews"));
402414
}
415+
416+
#[test]
417+
fn summary_len_truncated() {
418+
let comments = vec![ReviewComment {
419+
line_start: Some(1),
420+
line_end: 1,
421+
comment: "First comment".to_string(),
422+
path: "src/demo.cpp".to_string(),
423+
}];
424+
let clang_versions = ClangVersions {
425+
format_version: Some(PSEUDO_VERSION.clone()),
426+
tidy_version: Some(PSEUDO_VERSION),
427+
};
428+
let total_review_comments = 2;
429+
let summary_only = false;
430+
let long_patch = "a".repeat(u16::MAX as usize);
431+
let review_summary = ReviewComments {
432+
full_patch: long_patch,
433+
..Default::default()
434+
}
435+
.summarize(
436+
&clang_versions,
437+
&comments,
438+
total_review_comments,
439+
summary_only,
440+
);
441+
assert!(
442+
review_summary
443+
.contains("The full patch of fixes is too large to include in this summary.")
444+
);
445+
}
403446
}

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)