From 6494c5d7f455e57c39ff6d3d0ae0adead2f22a4b Mon Sep 17 00:00:00 2001 From: damocles Date: Sun, 1 Nov 2026 01:23:44 +0100 Subject: [PATCH] forge-notify: show unseen inline review comments in PR wakes --- docs/integrations/forge.md | 19 ++ hive-forge-notify/src/notify.rs | 329 ++++++++++++++++++++++++++++---- 2 files changed, 315 insertions(+), 33 deletions(-) diff --git a/docs/integrations/forge.md b/docs/integrations/forge.md index f39dbaea..a048a925 100644 --- a/docs/integrations/forge.md +++ b/docs/integrations/forge.md @@ -298,6 +298,25 @@ A review submitted with **no body** renders `reviewed by: ` in place of the `: ` line — deliberately worded to not collide with the meta-suffix `reviewer:` line (requested reviewers, below). +### Inline review comments + +A review's line comments belong to neither the review body nor the +`latest_comment_url` (a review with only inline comments has no such url +and otherwise lands as a bare `activity on PR`). So every PR wake fetches +the PR's reviews (`/pulls/N/reviews`, up to four pages of 50) and appends +the inline comments of those that are new to the agent as an +`inline review comments:` block, one `author [path:line]: text` entry +each (`[path]` when Forgejo dropped the position because the line left the +diff). A review counts as new when it was submitted strictly after the +thread's previously delivered `updated_at` (all of them without a +baseline, and any with no submit time, so nothing is hidden), is not the +agent's own, is not a `PENDING` draft, and has `comments_count > 0`. Only +the newest 5 such reviews are fetched, at most 10 comments are shown +(`(+N more ...)` for the rest), and each excerpt is truncated to 300 bytes +and heading-escaped like a body. Best effort: a failed fetch yields less +text, never a dropped wake. Cost: one reviews fetch per PR wake, plus one +per new commented review. + ### Merge/close vs a later comment A notification carrying a `latest_comment_url` normally takes the comment diff --git a/hive-forge-notify/src/notify.rs b/hive-forge-notify/src/notify.rs index 35e62857..2a7dc021 100644 --- a/hive-forge-notify/src/notify.rs +++ b/hive-forge-notify/src/notify.rs @@ -361,7 +361,7 @@ async fn format_notification( // comment stays on the comment path (see `state_change_is_current`). let is_fresh_state_change = state_change_is_current(¬if.state, notif.thread.updated_at, meta.subject.as_ref()); - if has_comment && !is_fresh_state_change { + let formatted = if has_comment && !is_fresh_state_change { let enrichment = CommentEnrichment { subject_api_url, prev_updated_at, @@ -377,38 +377,16 @@ async fn format_notification( ) .await } else { - // State-change path (merge/close/new/activity). A just-merged or - // closed subject keeps its pre-merge last comment on - // `latest_comment_url`, so `has_comment` is often true here. When - // that comment was genuinely posted AFTER the close — a comment - // racing the merge inside the state-change tolerance window — append - // it so it isn't lost (best of both worlds; see - // `docs/integrations/forge.md::Merge racing a comment`). The ordinary pre-merge - // last comment (created before `closed_at`) is left off. - let comment_tail = if has_comment { - fresh_post_close_comment_tail( - client, - source, - comment_api_url, - meta.subject.as_ref(), - own_login, - ) - .await - } else { - None - }; - // Same widened marker as the comment path (see that branch's - // comment), pre-fetched here because `format_state_change_notification` - // stays sync — mirrors how `comment_tail` above is already fetched - // before the sync formatter is called, same reason. - let earlier_updates = if let (Some(since), Some(current)) = - (prev_updated_at, notif.thread.updated_at) - { - fetch_unseen_earlier_events(client, source, subject_api_url, since, current, own_login) - .await - } else { - None - }; + let (comment_tail, earlier_updates) = state_change_extras( + client, + source, + has_comment.then_some(comment_api_url), + &meta, + subject_api_url, + prev_updated_at.zip(notif.thread.updated_at), + own_login, + ) + .await; format_state_change_notification( notif.thread.updated_at, ¬if.state, @@ -417,7 +395,57 @@ async fn format_notification( comment_tail, earlier_updates, ) + }; + // A review's inline comments ride neither the review body nor the + // comment url (a bodiless review has no `latest_comment_url` at all), + // so every PR wake gets the unseen ones appended. + let mut out = formatted?; + if is_pr { + out.push_str( + &fetch_inline_review_tail(client, source, subject_api_url, prev_updated_at, own_login) + .await, + ); } + Some(out) +} + +/// The two pre-fetches the sync state-change formatter needs. +/// +/// State-change path (merge/close/new/activity). A just-merged or +/// closed subject keeps its pre-merge last comment on +/// `latest_comment_url`, so a comment url is often present here. When +/// that comment was genuinely posted AFTER the close - a comment +/// racing the merge inside the state-change tolerance window - it is +/// appended so it isn't lost (best of both worlds; see +/// `docs/integrations/forge.md::Merge racing a comment`). The ordinary +/// pre-merge last comment (created before `closed_at`) is left off. +/// The earlier-updates count is the same widened marker as the comment +/// path; both are fetched here because `format_state_change_notification` +/// stays sync. +async fn state_change_extras( + client: &reqwest::Client, + source: &S, + comment_api_url: Option<&str>, + meta: &NotifMeta<'_>, + subject_api_url: &str, + since_and_current: Option<(&str, OffsetDateTime)>, + own_login: &str, +) -> (Option, Option) { + let comment_tail = match comment_api_url { + Some(url) => { + fresh_post_close_comment_tail(client, source, url, meta.subject.as_ref(), own_login) + .await + } + None => None, + }; + let earlier_updates = match since_and_current { + Some((since, current)) => { + fetch_unseen_earlier_events(client, source, subject_api_url, since, current, own_login) + .await + } + None => None, + }; + (comment_tail, earlier_updates) } /// Shared notification metadata extracted from the polled notification. @@ -872,6 +900,136 @@ async fn fresh_post_close_comment_tail( Some(format!("\n\ncomment by {author}: {excerpt}{mentions}")) } +/// Inline review comments rendered in one wake; the rest become a count. +const INLINE_COMMENTS_MAX: usize = 10; +/// Per-comment excerpt cap. Under [`BODY_TRUNCATE`]: one review carries many. +const INLINE_COMMENT_TRUNCATE: usize = 300; +/// Reviews whose inline comments are fetched for one wake (the newest). +const INLINE_REVIEWS_MAX: usize = 5; +/// Page size and page cap for the reviews list, so a long-lived PR's +/// newest reviews are not stranded behind the first page. +const REVIEWS_PAGE_SIZE: usize = 50; +const REVIEWS_PAGES_MAX: usize = 4; + +/// The PR's reviews endpoint from the notification subject URL. Forgejo +/// hands out the *issues* URL for a PR (`.../issues/N`); its reviews live +/// under `.../pulls/N/reviews`. `None` for any other shape. +fn reviews_api_url(subject_api_url: &str) -> Option { + let (head, number) = subject_api_url.rsplit_once("/issues/")?; + Some(format!("{head}/pulls/{number}/reviews")) +} + +/// `(review id, author)` of the reviews in `reviews` (the raw reviews +/// list, oldest first) that carry inline comments the agent has not been +/// woken for: submitted strictly after `since` (all of them without a +/// baseline, and any whose time is missing, so nothing is hidden), not +/// the agent's own, and not an unsubmitted `PENDING` draft. Newest +/// [`INLINE_REVIEWS_MAX`] only. +fn unseen_inline_reviews( + reviews: &[serde_json::Value], + since: Option, + own_login: &str, +) -> Vec<(i64, String)> { + let mut unseen: Vec<(i64, String)> = reviews + .iter() + .filter(|r| r["comments_count"].as_i64().unwrap_or(0) > 0) + .filter(|r| r["state"].as_str() != Some("PENDING")) + .filter(|r| { + let author = r["user"]["login"].as_str().unwrap_or(""); + own_login.is_empty() || author != own_login + }) + .filter( + |r| match (since, r["submitted_at"].as_str().and_then(parse_rfc3339)) { + (Some(since), Some(at)) => at > since, + _ => true, + }, + ) + .filter_map(|r| { + let id = r["id"].as_i64()?; + Some((id, r["user"]["login"].as_str().unwrap_or("?").to_owned())) + }) + .collect(); + let skip = unseen.len().saturating_sub(INLINE_REVIEWS_MAX); + unseen.drain(..skip); + unseen +} + +/// The `inline review comments:` block for the wake: one +/// `author [path:line]: text` entry per comment (`[path]` when Forgejo +/// dropped the position because the line left the diff), each excerpt +/// truncated + heading-escaped like a body, at most +/// [`INLINE_COMMENTS_MAX`] entries and a count for the rest. Empty when +/// there is nothing to show. +fn render_inline_comments(groups: &[(String, Vec)]) -> String { + let total: usize = groups.iter().map(|(_, c)| c.len()).sum(); + if total == 0 { + return String::new(); + } + let mut out = String::from("\n\ninline review comments:"); + let entries = groups + .iter() + .flat_map(|(author, comments)| comments.iter().map(move |c| (author, c))); + for (author, c) in entries.take(INLINE_COMMENTS_MAX) { + let path = c["path"].as_str().unwrap_or("?"); + let body = escape_md_headings(&truncate( + c["body"].as_str().unwrap_or("").trim(), + INLINE_COMMENT_TRUNCATE, + )) + .replace('\n', "\n "); + match c["position"].as_u64().filter(|&line| line > 0) { + Some(line) => write!(out, "\n {author} [{path}:{line}]: {body}").ok(), + None => write!(out, "\n {author} [{path}]: {body}").ok(), + }; + } + if total > INLINE_COMMENTS_MAX { + write!( + out, + "\n (+{} more - `hive-forge pr reviews` lists them all)", + total - INLINE_COMMENTS_MAX + ) + .ok(); + } + out +} + +/// Fetch + render the unseen inline review comments of a PR (see +/// [`unseen_inline_reviews`]). Best effort like every other enrichment: +/// any failed fetch just yields less (or no) text, never a dropped wake. +async fn fetch_inline_review_tail( + client: &reqwest::Client, + source: &S, + subject_api_url: &str, + prev_updated_at: Option<&str>, + own_login: &str, +) -> String { + let Some(reviews_url) = reviews_api_url(subject_api_url) else { + return String::new(); + }; + let mut reviews: Vec = Vec::new(); + for page in 1..=REVIEWS_PAGES_MAX { + let url = format!("{reviews_url}?page={page}&limit={REVIEWS_PAGE_SIZE}"); + let Some(batch) = fetch_json(client, &url, source).await else { + break; + }; + let Some(batch) = batch.as_array() else { + break; + }; + reviews.extend(batch.iter().cloned()); + if batch.len() < REVIEWS_PAGE_SIZE { + break; + } + } + let since = prev_updated_at.and_then(parse_rfc3339); + let mut groups = Vec::new(); + for (id, author) in unseen_inline_reviews(&reviews, since, own_login) { + let comments = fetch_json(client, &format!("{reviews_url}/{id}/comments"), source).await; + if let Some(comments) = comments.as_ref().and_then(serde_json::Value::as_array) { + groups.push((author, comments.clone())); + } + } + render_inline_comments(&groups) +} + /// Parse an RFC 3339 timestamp as Forgejo emits them /// (`2026-06-13T11:18:42+02:00` or `...Z`, optionally with fractional /// seconds). Returns `None` on any shape `time` doesn't recognise so @@ -1824,4 +1982,109 @@ mod tests { 2 ); } + + fn review(id: i64, user: &str, state: &str, n: i64, at: &str) -> serde_json::Value { + serde_json::json!({ + "id": id, + "user": {"login": user}, + "state": state, + "comments_count": n, + "submitted_at": at, + }) + } + + #[test] + fn reviews_api_url_swaps_issues_for_pulls() { + let subject = "/api/v1/repos/o/r/issues/42"; // lint:allow test input, not a tracker reference + assert_eq!( + reviews_api_url(subject).as_deref(), + Some("/api/v1/repos/o/r/pulls/42/reviews"), + ); + assert_eq!(reviews_api_url("/api/v1/repos/o/r"), None); + } + + #[test] + fn unseen_inline_reviews_keeps_only_new_foreign_submitted_commented() { + let since = parse_rfc3339("2026-10-30T10:00:00Z"); + let reviews = vec![ + review(1, "argus", "COMMENT", 2, "2026-10-30T09:00:00Z"), // seen + review(2, "argus", "COMMENT", 0, "2026-10-30T11:00:00Z"), // no inline + review(3, "me", "COMMENT", 3, "2026-10-30T11:00:00Z"), // own + review(4, "argus", "PENDING", 1, "2026-10-30T11:00:00Z"), // draft + review(5, "argus", "REQUEST_CHANGES", 1, "2026-10-30T12:00:00Z"), + ]; + assert_eq!( + unseen_inline_reviews(&reviews, since, "me"), + vec![(5, "argus".to_owned())] + ); + } + + #[test] + fn unseen_inline_reviews_without_baseline_or_time_hides_nothing() { + let reviews = vec![ + review(1, "argus", "COMMENT", 1, "2026-10-30T09:00:00Z"), + serde_json::json!({"id": 2, "user": {"login": "argus"}, "state": "COMMENT", "comments_count": 1}), + ]; + let ids: Vec = unseen_inline_reviews(&reviews, None, "me") + .into_iter() + .map(|(id, _)| id) + .collect(); + assert_eq!(ids, vec![1, 2]); + // A missing submit time is shown even against a baseline. + let since = parse_rfc3339("2026-10-30T10:00:00Z"); + let ids: Vec = unseen_inline_reviews(&reviews, since, "me") + .into_iter() + .map(|(id, _)| id) + .collect(); + assert_eq!(ids, vec![2]); + } + + #[test] + fn unseen_inline_reviews_keeps_the_newest_few() { + let reviews: Vec<_> = (1..=8) + .map(|i| review(i, "argus", "COMMENT", 1, "2026-10-30T11:00:00Z")) + .collect(); + let ids: Vec = unseen_inline_reviews(&reviews, None, "me") + .into_iter() + .map(|(id, _)| id) + .collect(); + assert_eq!(ids, vec![4, 5, 6, 7, 8]); + } + + #[test] + fn render_inline_comments_formats_path_line_and_fallback() { + let groups = vec![( + "argus".to_owned(), + vec![ + serde_json::json!({"path": "src/a.rs", "position": 42, "body": "off by one"}), + serde_json::json!({"path": "src/b.rs", "position": 0, "body": "line left the diff"}), + serde_json::json!({"path": "src/c.rs", "body": "# heading\nsecond line"}), + ], + )]; + assert_eq!( + render_inline_comments(&groups), + "\n\ninline review comments:\ + \n argus [src/a.rs:42]: off by one\ + \n argus [src/b.rs]: line left the diff\ + \n argus [src/c.rs]: \\# heading\n second line" + ); + } + + #[test] + fn render_inline_comments_empty_is_empty() { + assert_eq!(render_inline_comments(&[]), ""); + assert_eq!(render_inline_comments(&[("argus".to_owned(), vec![])]), ""); + } + + #[test] + fn render_inline_comments_caps_entries_and_excerpts() { + let comments: Vec<_> = (0..13) + .map(|i| serde_json::json!({"path": "f.rs", "position": i + 1, "body": "x".repeat(1000)})) + .collect(); + let out = render_inline_comments(&[("argus".to_owned(), comments)]); + assert_eq!(out.matches("argus [f.rs:").count(), INLINE_COMMENTS_MAX); + assert!(out.ends_with("(+3 more - `hive-forge pr reviews` lists them all)")); + // Each excerpt is cut, not the full 1000 chars. + assert!(out.len() < INLINE_COMMENTS_MAX * (INLINE_COMMENT_TRUNCATE + 60) + 200); + } }