From e3f0f998f4b75c00b9a05a3b0cb39d470b6d5633 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Sat, 10 Jan 2026 20:54:53 +0100 Subject: [PATCH 1/3] Add test for `summary_comment_template` output validation Add a golden-output test for the Reviews markdown so follow-up refactors show the exact formatting changes. --- .../src/features/summary_comment.rs | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/webhook_features/src/features/summary_comment.rs b/webhook_features/src/features/summary_comment.rs index f20c888..4df4010 100644 --- a/webhook_features/src/features/summary_comment.rs +++ b/webhook_features/src/features/summary_comment.rs @@ -518,7 +518,31 @@ async fn get_llm_check(llm_diff_pr: &str, llm_token: &str) -> Result #[cfg(test)] mod tests { use super::*; + use chrono::TimeZone; + #[test] + fn test_summary_comment_template() { + let reviews = vec![Review { + user: "user".to_string(), + ack_type: AckType::Ack, + url: "https://example.com/review".to_string(), + date: chrono::Utc.with_ymd_and_hms(2026, 1, 2, 0, 0, 0).unwrap(), + }]; + + let text = summary_comment_template(reviews); + assert_eq!( + text, + r#" +### Reviews +See [the guideline](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review) for information on the review process. +| Type | Reviewers | +| ---- | --------- | +| ACK | [user](https://example.com/review) | + +If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore. +"# + ); + } struct TestCase { comment: &'static str, expected: Option, From 86c7822aa19b2fcf488f3bb1442a4c2c312fa681 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Sat, 10 Jan 2026 11:16:02 +0100 Subject: [PATCH 2/3] Extract review collection Move review parsing and selection into `collect_user_reviews()` to isolate the review selection logic for follow-up history rendering. --- .../src/features/summary_comment.rs | 84 ++++++++++--------- 1 file changed, 46 insertions(+), 38 deletions(-) diff --git a/webhook_features/src/features/summary_comment.rs b/webhook_features/src/features/summary_comment.rs index 4df4010..3eea0b8 100644 --- a/webhook_features/src/features/summary_comment.rs +++ b/webhook_features/src/features/summary_comment.rs @@ -183,6 +183,51 @@ struct GitHubReviewComment { date: chrono::DateTime, } +fn collect_user_reviews( + all_comments: Vec, + pr_author: &str, + head_commit: &str, +) -> Vec { + let mut user_reviews: HashMap> = HashMap::new(); // Need to store all acks per user to avoid duplicates + + for comment in all_comments.into_iter() { + if comment.user == pr_author { + continue; + } + if let Some(ac) = parse_review(&comment.body) { + let v = user_reviews.entry(comment.user.clone()).or_default(); + let has_current_head = ac.commit.is_some_and(|c| head_commit.starts_with(&c)); + v.push(Review { + user: comment.user.clone(), + ack_type: if comment.body.contains(BOT_SKIP_TAG) { + AckType::Ignored + } else if ac.ack_type == AckType::Ack && !has_current_head { + AckType::StaleAck + } else { + ac.ack_type + }, + url: comment.url, + date: comment.date, + }); + } + } + + user_reviews + .into_iter() + .map(|e| { + let e = e.1; + if let Some(ack) = e.iter().find(|r| r.ack_type == AckType::Ack) { + // Prefer ACK commit_hash over anything, to match the behavior of + // https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/f9b845614f7aecb9423d0621375e1bad17f92fde/github-merge.py#L208 + ack.clone() + } else { + // Fallback to the most recent comment, otherwise + e.into_iter().max_by_key(|r| r.date).unwrap() + } + }) + .collect::>() +} + async fn refresh_summary_comment( ctx: &Context, repo: Repository, @@ -283,8 +328,6 @@ For details see: https://corecheck.dev/{owner}/{repo}/pulls/{pull_num}. let head_commit = pr.head.sha; - let mut user_reviews: HashMap> = HashMap::new(); // Need to store all acks per user to avoid duplicates - println!( " ... Refresh of {num} comments from {url}.", num = all_comments.len(), @@ -292,42 +335,7 @@ For details see: https://corecheck.dev/{owner}/{repo}/pulls/{pull_num}. ); let pr_author = pr.user.unwrap().login; - for comment in all_comments.into_iter() { - if comment.user == pr_author { - continue; - } - if let Some(ac) = parse_review(&comment.body) { - let v = user_reviews.entry(comment.user.clone()).or_default(); - let has_current_head = ac.commit.is_some_and(|c| head_commit.starts_with(&c)); - v.push(Review { - user: comment.user.clone(), - ack_type: if comment.body.contains(BOT_SKIP_TAG) { - AckType::Ignored - } else if ac.ack_type == AckType::Ack && !has_current_head { - AckType::StaleAck - } else { - ac.ack_type - }, - url: comment.url, - date: comment.date, - }); - } - } - - let user_reviews = user_reviews - .into_iter() - .map(|e| { - let e = e.1; - if let Some(ack) = e.iter().find(|r| r.ack_type == AckType::Ack) { - // Prefer ACK commit_hash over anything, to match the behavior of - // https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/f9b845614f7aecb9423d0621375e1bad17f92fde/github-merge.py#L208 - ack.clone() - } else { - // Fallback to the most recent comment, otherwise - e.into_iter().max_by_key(|r| r.date).unwrap() - } - }) - .collect::>(); + let user_reviews = collect_user_reviews(all_comments, &pr_author, &head_commit); let max_ack_date = user_reviews .iter() From 3c7e659d061bc8d0ce873c4b9fde9d8c639e2ee7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Sat, 10 Jan 2026 11:36:59 +0100 Subject: [PATCH 3/3] Link past ACKs to a collapsible history section Add a Review history details block under the Reviews table that lists every non-ignored review event (type, reviewer, date) linked to the original comment. Inline superscript/brace links were too verbose, so the history now lives in a separate expandable section instead. --- .../src/features/summary_comment.rs | 64 ++++++++++++++++--- 1 file changed, 55 insertions(+), 9 deletions(-) diff --git a/webhook_features/src/features/summary_comment.rs b/webhook_features/src/features/summary_comment.rs index 3eea0b8..196b87e 100644 --- a/webhook_features/src/features/summary_comment.rs +++ b/webhook_features/src/features/summary_comment.rs @@ -118,7 +118,7 @@ impl Feature for SummaryCommentFeature { const BOT_SKIP_TAG: &str = ""; -fn summary_comment_template(reviews: Vec) -> String { +fn summary_comment_template(reviews: Vec, history: Vec) -> String { let review_url = "https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review"; let mut comment = format!( r#" @@ -159,13 +159,23 @@ See [the guideline]({review_url}) for information on the review process. ack_type.as_str(), users .iter() - .map(|(user, url, _)| format!("[{user}]({url})")) + .map(|(user, url, _)| format_ack(user, url)) .collect::>() .join(", ") ); } } + if !history.is_empty() { + comment += "\n
Review history\n\n"; + comment += "| Type | Reviewer | Date |\n"; + comment += "| ---- | -------- | ---- |\n"; + for Review { ack_type, user, url, date } in history { + comment += &format!("| {} | {} | {} |\n", ack_type.as_str(), format_ack(&user, &url), date.format("%F")); + } + comment += "\n
\n"; + } + comment += "\n"; comment += "If your review is incorrectly listed, please copy-paste "; comment += "<!--meta-tag:bot-skip-->" /* BOT_SKIP_TAG */; @@ -176,6 +186,8 @@ See [the guideline]({review_url}) for information on the review process. comment } +fn format_ack(user: &String, url: &String) -> String { format!("[{user}]({url})") } + struct GitHubReviewComment { user: String, url: String, @@ -187,7 +199,7 @@ fn collect_user_reviews( all_comments: Vec, pr_author: &str, head_commit: &str, -) -> Vec { +) -> (Vec, Vec) { let mut user_reviews: HashMap> = HashMap::new(); // Need to store all acks per user to avoid duplicates for comment in all_comments.into_iter() { @@ -212,7 +224,15 @@ fn collect_user_reviews( } } - user_reviews + let mut history = user_reviews + .values() + .flatten() + .filter(|r| r.ack_type != AckType::Ignored) + .cloned() + .collect::>(); + history.sort_by_key(|r| r.date); + + let reviews = user_reviews .into_iter() .map(|e| { let e = e.1; @@ -225,7 +245,9 @@ fn collect_user_reviews( e.into_iter().max_by_key(|r| r.date).unwrap() } }) - .collect::>() + .collect::>(); + + (reviews, history) } async fn refresh_summary_comment( @@ -335,7 +357,7 @@ For details see: https://corecheck.dev/{owner}/{repo}/pulls/{pull_num}. ); let pr_author = pr.user.unwrap().login; - let user_reviews = collect_user_reviews(all_comments, &pr_author, &head_commit); + let (user_reviews, history) = collect_user_reviews(all_comments, &pr_author, &head_commit); let max_ack_date = user_reviews .iter() @@ -378,7 +400,7 @@ For details see: https://corecheck.dev/{owner}/{repo}/pulls/{pull_num}. .map(|r| r.user.clone()) .collect::>(); - let comment = summary_comment_template(user_reviews); + let comment = summary_comment_template(user_reviews, history); util::update_metadata_comment( &issues_api, &mut cmt, @@ -529,7 +551,7 @@ mod tests { use chrono::TimeZone; #[test] - fn test_summary_comment_template() { + fn test_summary_comment_template_includes_history() { let reviews = vec![Review { user: "user".to_string(), ack_type: AckType::Ack, @@ -537,7 +559,22 @@ mod tests { date: chrono::Utc.with_ymd_and_hms(2026, 1, 2, 0, 0, 0).unwrap(), }]; - let text = summary_comment_template(reviews); + let history = vec![ + Review { + user: "user".to_string(), + ack_type: AckType::ConceptAck, + url: "https://example.com/review/old".to_string(), + date: chrono::Utc.with_ymd_and_hms(2026, 1, 1, 0, 0, 0).unwrap(), + }, + Review { + user: "user".to_string(), + ack_type: AckType::Ack, + url: "https://example.com/review".to_string(), + date: chrono::Utc.with_ymd_and_hms(2026, 1, 2, 0, 0, 0).unwrap(), + }, + ]; + + let text = summary_comment_template(reviews, history); assert_eq!( text, r#" @@ -547,6 +584,15 @@ See [the guideline](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING. | ---- | --------- | | ACK | [user](https://example.com/review) | +
Review history + +| Type | Reviewer | Date | +| ---- | -------- | ---- | +| Concept ACK | [user](https://example.com/review/old) | 2026-01-01 | +| ACK | [user](https://example.com/review) | 2026-01-02 | + +
+ If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore. "# );