Watch
0
0
Fork
You've already forked hyperhive
0

forge notify: a review stays in the todo's inline block until the agent answers it, not just until it was delivered once

The todo row is last-write-wins, so a review delivered in one version of the row disappeared when a later comment rewrote it, because the inline tail only listed reviews newer than the last delivery. The baseline is now the agent's own last review or comment on the thread.
This commit is contained in:
damocles 2026-11-11 18:58:42 +01:00 • committed by mara
commit c5729b4124
2 changed files with 104 additions and 30 deletions

View file

@ -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 `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 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, 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 `inline review comments (read them with `hive-forge pr reviews N`):` block
saying where they sit: one `author: path:line, path, ...` line per review saying where they sit: one `author: path:line, path, ...` line per review
author (bare `path` when Forgejo dropped the position because the line left 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 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. 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 A review counts as unanswered when it landed strictly after the agent's own
delivered `updated_at`. Without a baseline every review counts, and so does last activity on the thread: its newest review or comment, read from the same
one with no submit time, so nothing disappears. It doesn't count when the reviews list and from `/issues/N/comments`. The baseline is the agent's own
agent wrote it, when it's a `PENDING` draft, or when `comments_count` is 0. 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 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 (`(+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 never drops the wake. Cost: one reviews fetch and one comments fetch per PR
commented review. wake, plus one per unanswered commented review.
### Merge/close vs a later comment ### Merge/close vs a later comment

View file

@ -398,13 +398,10 @@ async fn format_notification<S: Source>(
}; };
// A review's inline comments ride neither the review body nor the // A review's inline comments ride neither the review body nor the
// comment url (a bodiless review has no `latest_comment_url` at all), // 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?; let mut out = formatted?;
if is_pr { if is_pr {
out.push_str( out.push_str(&fetch_inline_review_tail(client, source, subject_api_url, own_login).await);
&fetch_inline_review_tail(client, source, subject_api_url, prev_updated_at, own_login)
.await,
);
} }
Some(out) 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 /// `(review id, author)` of the reviews in `reviews` (the raw reviews
/// list, oldest first) that carry inline comments the agent has not been /// list, oldest first) that carry inline comments the agent has not answered:
/// woken for: submitted strictly after `since` (all of them without a /// 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 /// baseline, and any whose time is missing, so nothing is hidden), not
/// the agent's own, and not an unsubmitted `PENDING` draft. Newest /// the agent's own, and not an unsubmitted `PENDING` draft. Newest
/// [`INLINE_REVIEWS_MAX`] only. /// [`INLINE_REVIEWS_MAX`] only.
@ -996,34 +993,77 @@ fn render_inline_comments(pr_number: &str, groups: &[(String, Vec<serde_json::Va
out out
} }
/// Fetch + render the unseen inline review comments of a PR (see /// The pages of a list endpoint, concatenated, at most [`REVIEWS_PAGES_MAX`]
/// [`unseen_inline_reviews`]). Best effort like every other enrichment: /// of [`REVIEWS_PAGE_SIZE`], oldest first as Forgejo returns them. Stops at
/// any failed fetch just yields less (or no) text, never a dropped wake. /// the first failed page, so a partial list is what a flaky forge gives.
async fn fetch_inline_review_tail<S: Source>( async fn fetch_list_pages<S: Source>(
client: &reqwest::Client, client: &reqwest::Client,
source: &S, source: &S,
subject_api_url: &str, url: &str,
prev_updated_at: Option<&str>, ) -> Vec<serde_json::Value> {
own_login: &str, let mut items: Vec<serde_json::Value> = Vec::new();
) -> String {
let Some((reviews_url, pr_number)) = reviews_api_url(subject_api_url) else {
return String::new();
};
let mut reviews: Vec<serde_json::Value> = Vec::new();
for page in 1..=REVIEWS_PAGES_MAX { for page in 1..=REVIEWS_PAGES_MAX {
let url = format!("{reviews_url}?page={page}&limit={REVIEWS_PAGE_SIZE}"); let page_url = format!("{url}?page={page}&limit={REVIEWS_PAGE_SIZE}");
let Some(batch) = fetch_json(client, &url, source).await else { let Some(batch) = fetch_json(client, &page_url, source).await else {
break; break;
}; };
let Some(batch) = batch.as_array() else { let Some(batch) = batch.as_array() else {
break; break;
}; };
reviews.extend(batch.iter().cloned()); items.extend(batch.iter().cloned());
if batch.len() < REVIEWS_PAGE_SIZE { if batch.len() < REVIEWS_PAGE_SIZE {
break; 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<OffsetDateTime> {
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::<Vec<_>>()
};
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<S: Source>(
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(); let mut groups = Vec::new();
for (id, author) in unseen_inline_reviews(&reviews, since, own_login) { for (id, author) in unseen_inline_reviews(&reviews, since, own_login) {
let comments = fetch_json(client, &format!("{reviews_url}/{id}/comments"), source).await; 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] #[test]
fn unseen_inline_reviews_without_baseline_or_time_hides_nothing() { fn unseen_inline_reviews_without_baseline_or_time_hides_nothing() {
let reviews = vec![ let reviews = vec![