diff --git a/docs/4-implementation/agent-tools/forge-notify.md b/docs/4-implementation/agent-tools/forge-notify.md index 551600181..b428cfce6 100644 --- a/docs/4-implementation/agent-tools/forge-notify.md +++ b/docs/4-implementation/agent-tools/forge-notify.md @@ -178,21 +178,27 @@ 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 would otherwise land as a bare `activity on PR`. Every PR wake therefore fetches the PR's reviews (`/pulls/N/reviews`, up to four pages of 50) and, -for the reviews new to the agent, appends an +for the reviews the agent hasn't answered, appends an `inline review comments (read them with `hive-forge pr reviews N`):` block saying where they sit: one `author: path:line, path, ...` line per review author (bare `path` when Forgejo dropped the position because the line left the diff; control characters in a path become spaces). The comment text stays out of the wake, and the agent reads the full review with the CLI. -A review counts as new when it landed strictly after the thread's previously -delivered `updated_at`. Without a baseline every review counts, and so does -one with no submit time, so nothing disappears. It doesn't count when the -agent wrote it, when it's a `PENDING` draft, or when `comments_count` is 0. +A review counts as unanswered when it landed strictly after the agent's own +last activity on the thread: its newest review or comment, read from the same +reviews list and from `/issues/N/comments`. The baseline is the agent's own +activity and not the last delivery on purpose. The todo row is last-write-wins, +so a later comment rewrites the row and drops a review delivered in an earlier +version of it, and treating that review as delivered would hide it from an agent +that never read it. Without a baseline (an agent that has neither reviewed nor +commented) every review counts, and so does one with no submit time, so nothing +disappears. It doesn't count when the agent wrote it, when it's a `PENDING` +draft, or when `comments_count` is 0. The notifier fetches the newest 5 such reviews and lists at most 10 locations (`(+N more)` for the rest). Best effort: a failed fetch yields less text and -never drops the wake. Cost: one reviews fetch per PR wake, plus one per new -commented review. +never drops the wake. Cost: one reviews fetch and one comments fetch per PR +wake, plus one per unanswered commented review. ### Merge/close vs a later comment diff --git a/hive-forge-notify/src/notify.rs b/hive-forge-notify/src/notify.rs index 2b3315019..71fffa5d3 100644 --- a/hive-forge-notify/src/notify.rs +++ b/hive-forge-notify/src/notify.rs @@ -398,13 +398,10 @@ async fn format_notification( }; // 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. + // so every PR wake gets the ones the agent has not answered 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, - ); + out.push_str(&fetch_inline_review_tail(client, source, subject_api_url, own_login).await); } Some(out) } @@ -918,8 +915,8 @@ fn reviews_api_url(subject_api_url: &str) -> Option<(String, &str)> { } /// `(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 +/// list, oldest first) that carry inline comments the agent has not answered: +/// submitted strictly after `since`, its own last activity (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. @@ -996,34 +993,77 @@ fn render_inline_comments(pr_number: &str, groups: &[(String, Vec( +/// The pages of a list endpoint, concatenated, at most [`REVIEWS_PAGES_MAX`] +/// of [`REVIEWS_PAGE_SIZE`], oldest first as Forgejo returns them. Stops at +/// the first failed page, so a partial list is what a flaky forge gives. +async fn fetch_list_pages( client: &reqwest::Client, source: &S, - subject_api_url: &str, - prev_updated_at: Option<&str>, - own_login: &str, -) -> String { - let Some((reviews_url, pr_number)) = reviews_api_url(subject_api_url) else { - return String::new(); - }; - let mut reviews: Vec = Vec::new(); + url: &str, +) -> Vec { + let mut items: 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 { + let page_url = format!("{url}?page={page}&limit={REVIEWS_PAGE_SIZE}"); + let Some(batch) = fetch_json(client, &page_url, source).await else { break; }; let Some(batch) = batch.as_array() else { break; }; - reviews.extend(batch.iter().cloned()); + items.extend(batch.iter().cloned()); if batch.len() < REVIEWS_PAGE_SIZE { break; } } - let since = prev_updated_at.and_then(parse_rfc3339); + items +} + +/// When the agent last acted on the thread: its newest review (`reviews`, +/// by `submitted_at`) or comment (`comments`, by `created_at`). `None` for an +/// agent that has done neither, or whose login is unknown. +fn own_last_activity( + reviews: &[serde_json::Value], + comments: &[serde_json::Value], + own_login: &str, +) -> Option { + if own_login.is_empty() { + return None; + } + let times = |items: &[serde_json::Value], field: &'static str| { + items + .iter() + .filter(|i| i["user"]["login"].as_str() == Some(own_login)) + .filter_map(|i| i[field].as_str().and_then(parse_rfc3339)) + .collect::>() + }; + times(reviews, "submitted_at") + .into_iter() + .chain(times(comments, "created_at")) + .max() +} + +/// Fetch + render the inline review comments of a PR that the agent has not +/// answered (see [`unseen_inline_reviews`]). The baseline is the agent's own +/// last activity on the thread, **not** the last delivery: the todo row is +/// last-write-wins, so a review delivered in one version is gone from the row +/// when a later event rewrites it, and a baseline of "last delivered" would +/// then treat that review as seen though the agent never read it. Best +/// effort like every other enrichment: any failed fetch just shortens the +/// block. +async fn fetch_inline_review_tail( + client: &reqwest::Client, + source: &S, + subject_api_url: &str, + own_login: &str, +) -> String { + let Some((reviews_url, pr_number)) = reviews_api_url(subject_api_url) else { + return String::new(); + }; + let reviews = fetch_list_pages(client, source, &reviews_url).await; + // The agent's comments count as an answer too, and an author who never + // reviews its own PR has no review to mark it by. + let comments = fetch_list_pages(client, source, &format!("{subject_api_url}/comments")).await; + let since = own_last_activity(&reviews, &comments, own_login); 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; @@ -2023,6 +2063,34 @@ mod tests { ); } + #[test] + fn the_agents_own_review_or_comment_is_what_marks_a_review_answered() { + let reviews = vec![ + review(1, "me", "REQUEST_CHANGES", 1, "2026-10-30T10:00:00Z"), + review(2, "mara", "REQUEST_CHANGES", 3, "2026-10-30T11:00:00Z"), + ]; + let comment = + |user: &str, at: &str| serde_json::json!({"user": {"login": user}, "created_at": at}); + // Mara's review came after the agent's own: it is still unanswered, + // however many later comments rewrote the row since (iris' here). + let comments = vec![comment("iris", "2026-10-30T11:30:00Z")]; + let since = own_last_activity(&reviews, &comments, "me"); + assert_eq!(since, parse_rfc3339("2026-10-30T10:00:00Z")); + assert_eq!( + unseen_inline_reviews(&reviews, since, "me"), + vec![(2, "mara".to_owned())] + ); + // The agent answered by commenting afterwards: nothing left to show. + let answered = vec![comment("me", "2026-10-30T12:00:00Z")]; + let since = own_last_activity(&reviews, &answered, "me"); + assert_eq!(since, parse_rfc3339("2026-10-30T12:00:00Z")); + assert!(unseen_inline_reviews(&reviews, since, "me").is_empty()); + // An author with neither review nor comment has no baseline, and an + // unknown login never matches a stranger's activity. + assert_eq!(own_last_activity(&reviews[1..], &[], "me"), None); + assert_eq!(own_last_activity(&reviews, &comments, ""), None); + } + #[test] fn unseen_inline_reviews_without_baseline_or_time_hides_nothing() { let reviews = vec![