diff --git a/hive-forge-notify/src/notify.rs b/hive-forge-notify/src/notify.rs index c80afeb7..7c9a0e95 100644 --- a/hive-forge-notify/src/notify.rs +++ b/hive-forge-notify/src/notify.rs @@ -260,6 +260,7 @@ async fn format_notification( source: &S, notif: &PolledNotification, own_login: &str, + prev_updated_at: Option<&str>, ) -> Option { let subj = notif.thread.subject.as_ref(); let title = subj.and_then(|s| s.title.as_deref()).unwrap_or("?"); @@ -339,12 +340,17 @@ async fn format_notification( 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 enrichment = CommentEnrichment { + subject_api_url, + prev_updated_at, + }; format_comment_notification( client, source, &meta, comment_api_url, comment_html_url, + &enrichment, own_login, ) .await @@ -369,12 +375,25 @@ async fn format_notification( } 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 + }; format_state_change_notification( notif.thread.updated_at, ¬if.state, &meta, own_login, comment_tail, + earlier_updates, ) } } @@ -426,6 +445,24 @@ fn build_meta_suffix(subject: Option<&serde_json::Value>, is_pr: bool) -> String out } +/// The extra context `format_comment_notification` needs, beyond +/// `NotifMeta`, only to power the "earlier unseen update" marker on the +/// plain-comment path — see [`fetch_unseen_earlier_events`]. The +/// state-change path wants the same two values, but fetches them itself in +/// `format_notification` before calling the (sync) state-change formatter, +/// so it never needs this bundle — kept as its own small struct rather than +/// folded into `NotifMeta` for that reason, not because the other path has +/// no use for the underlying data. +struct CommentEnrichment<'a> { + subject_api_url: &'a str, + /// The thread's previously-delivered raw `updated_at`, from the + /// poller's in-process dedupe map. `None` when this is the first + /// delivery we've made for the thread this process's lifetime — in + /// that case there's no earlier baseline to diff comments against, + /// so no marker is computed. + prev_updated_at: Option<&'a str>, +} + /// Format a notification triggered by a new comment or review submission. async fn format_comment_notification( client: &reqwest::Client, @@ -433,6 +470,7 @@ async fn format_comment_notification( meta: &NotifMeta<'_>, comment_api_url: &str, comment_html_url: &str, + enrichment: &CommentEnrichment<'_>, own_login: &str, ) -> Option { let payload = fetch_json(client, comment_api_url, source).await; @@ -504,11 +542,99 @@ async fn format_comment_notification( let mut out = format!( "[{kind}] {title}\nurl: {url}\n\n{author}: {body_for_embed}{truncated_mentions}" ); + // Forgejo's notification API tracks one row per subject with a + // single `latest_comment_url`, so anything else that happened on + // the same thread since we last looked — an earlier comment, a + // label, a close/reopen, a review — is otherwise never individually + // surfaced; the blurb silently shows only the newest comment. Flag + // a count rather than rendering more text (see the forge-notify + // hidden-comment issue for why), over the full timeline rather than + // just comments (widened per mara's review call: "properly check + // the timeline"). + if let (Some(since), Some(current)) = ( + enrichment.prev_updated_at, + payload + .as_ref() + .and_then(|c| c["created_at"].as_str()) + .and_then(parse_rfc3339), + ) { + let unseen = fetch_unseen_earlier_events( + client, + source, + enrichment.subject_api_url, + since, + current, + own_login, + ) + .await; + out.push_str(&earlier_updates_marker(unseen)); + } out.push_str(meta_suffix); Some(out) } } +/// Count entries in `events` (raw JSON array from the issue/PR **timeline** +/// endpoint — comments AND state-change events: label, assign, close/reopen, +/// review, `pull_push`, …) that landed strictly after `since` and strictly +/// before `current`, excluding any authored by `own_login`. Deliberately +/// type-agnostic — every timeline entry the API returns is counted the same +/// way, per mara's review call ("count everything") rather than singling +/// out any one type as noise. Pure for unit testing; +/// [`fetch_unseen_earlier_events`] does the network fetch and calls this on +/// the result. +fn count_unseen_earlier_events( + events: &[serde_json::Value], + since: OffsetDateTime, + current: OffsetDateTime, + own_login: &str, +) -> usize { + events + .iter() + .filter(|e| { + let created = e["created_at"].as_str().and_then(parse_rfc3339); + let author = e["user"]["login"].as_str().unwrap_or(""); + matches!(created, Some(t) if t > since && t < current) + && (own_login.is_empty() || author != own_login) + }) + .count() +} + +/// Fetch the subject's full timeline (comments + every state-change event) +/// and count how many entries landed strictly between `since` (the thread's +/// previously-delivered raw `updated_at`) and `current` (the event now being +/// rendered) — everything Forgejo's one-row-per-subject notification +/// silently drops between polls, comment or not. `None` on any +/// fetch/parse failure or an unparseable `since`; the caller just omits the +/// marker rather than blocking delivery on it. +async fn fetch_unseen_earlier_events( + client: &reqwest::Client, + source: &S, + subject_api_url: &str, + since: &str, + current: OffsetDateTime, + own_login: &str, +) -> Option { + let since = parse_rfc3339(since)?; + let list = fetch_json(client, &format!("{subject_api_url}/timeline"), source).await?; + let arr = list.as_array()?; + Some(count_unseen_earlier_events(arr, since, current, own_login)) +} + +/// The trailing `(+N earlier update(s) since your last visit)` marker for +/// [`fetch_unseen_earlier_events`]'s result — empty string for `None` (fetch +/// failed) or `Some(0)` (nothing hidden), shared by both delivery paths so +/// the wording can't drift between them. +fn earlier_updates_marker(unseen: Option) -> String { + match unseen { + Some(n) if n > 0 => { + let plural = if n == 1 { "" } else { "s" }; + format!("\n\n(+{n} earlier update{plural} since your last visit)") + } + _ => String::new(), + } +} + /// Format a notification triggered by creation or state change of the subject. /// /// Returns `None` for an agent's own *creation* (it opened the issue/PR) — @@ -521,6 +647,7 @@ fn format_state_change_notification( meta: &NotifMeta<'_>, own_login: &str, comment_tail: Option, + earlier_updates: Option, ) -> Option { // Classification uses the raw `subject.state` string extracted in // `parse_notification` — Forgejo returns "open" / "closed" / "merged" @@ -608,6 +735,11 @@ fn format_state_change_notification( if let Some(tail) = comment_tail { out.push_str(&tail); } + // Same widened "you might be missing something" marker the comment path + // carries — a state-change notification (e.g. one merge) can just as + // easily hide several intervening labels/comments/reopens as a comment + // notification can hide earlier comments. + out.push_str(&earlier_updates_marker(earlier_updates)); out.push_str(meta_suffix); Some(out) } @@ -914,13 +1046,20 @@ pub async fn poll_once( // shows up unread in the next `?all=false` poll. Skip unless its // `updated_at` advanced since the version we last delivered (i.e. // genuinely new activity). See the `delivered` note in `run`. + // + // Captured before the `should_deliver` check consumes `delivered` + // (which only borrows) so the formatter can diff the thread's + // comment list against the version we last saw — see + // `CommentEnrichment`. + let prev_updated_at = delivered.get(id.as_str()).cloned(); let updated_at = notif.updated_at.clone(); if !should_deliver(delivered, &id, &updated_at) { debug!(%id, "forge_notify: skipping (already delivered this version)"); continue; } - let body_opt = format_notification(client, source, notif, own_login).await; + let body_opt = + format_notification(client, source, notif, own_login, prev_updated_at.as_deref()).await; // None means self-echo — mark read silently, no delivery. let Some(body) = body_opt else { @@ -1406,13 +1545,17 @@ mod tests { fn state_change_drops_self_authored_creation() { // No timestamps ⇒ treated as a creation; poster login == own_login. let meta = state_change_meta(serde_json::json!({ "user": { "login": "damocles" } })); - assert!(format_state_change_notification(None, "open", &meta, "damocles", None).is_none()); + assert!( + format_state_change_notification(None, "open", &meta, "damocles", None, None).is_none() + ); } #[test] fn state_change_keeps_other_authored_creation() { let meta = state_change_meta(serde_json::json!({ "user": { "login": "someone-else" } })); - assert!(format_state_change_notification(None, "open", &meta, "damocles", None).is_some()); + assert!( + format_state_change_notification(None, "open", &meta, "damocles", None, None).is_some() + ); } #[test] @@ -1426,7 +1569,8 @@ mod tests { })); let event = parse_rfc3339("2026-06-22T16:00:00Z"); assert!( - format_state_change_notification(event, "closed", &meta, "damocles", None).is_some() + format_state_change_notification(event, "closed", &meta, "damocles", None, None) + .is_some() ); } @@ -1471,6 +1615,7 @@ mod tests { &meta, "damocles", Some("\n\ncomment by argus: nice, merging".to_owned()), + None, ) .expect("merge notification must render"); assert!(out.contains("comment by argus: nice, merging")); @@ -1491,12 +1636,129 @@ mod tests { "user": { "login": "someone-else" }, "body": "the PR description", })); - let new_out = format_state_change_notification(None, "open", &meta, "damocles", None) + let new_out = format_state_change_notification(None, "open", &meta, "damocles", None, None) .expect("open notification must render"); assert!(!new_out.contains("the PR description")); - let merged_out = format_state_change_notification(None, "merged", &meta, "damocles", None) - .expect("merge notification must render"); + let merged_out = + format_state_change_notification(None, "merged", &meta, "damocles", None, None) + .expect("merge notification must render"); assert!(!merged_out.contains("the PR description")); } + + /// The widened marker (per mara's review call: "count everything") must + /// render on the state-change path too, not just the comment path — a merge/close + /// notification can hide just as many intervening events as a comment + /// notification can. + #[test] + fn format_state_change_appends_earlier_updates_marker() { + let meta = state_change_meta(serde_json::json!({ + "user": { "login": "someone-else" }, + })); + let out = + format_state_change_notification(None, "merged", &meta, "damocles", None, Some(3)) + .expect("merge notification must render"); + assert!(out.contains("(+3 earlier updates since your last visit)")); + } + + #[test] + fn format_state_change_omits_marker_when_nothing_unseen() { + let meta = state_change_meta(serde_json::json!({ + "user": { "login": "someone-else" }, + })); + let out = format_state_change_notification(None, "merged", &meta, "damocles", None, None) + .expect("merge notification must render"); + assert!(!out.contains("earlier update")); + } + + /// Regression coverage for the earlier-unseen-update gap: + /// `count_unseen_earlier_events` is the pure core of that marker. + #[test] + fn count_unseen_earlier_events_counts_only_the_gap() { + let since = parse_rfc3339("2026-06-13T10:00:00Z").unwrap(); + let current = parse_rfc3339("2026-06-13T17:01:00Z").unwrap(); + let events = [ + // Before `since` — already seen on a prior delivery, not counted. + serde_json::json!({ "created_at": "2026-06-13T09:00:00Z", "user": { "login": "mara" } }), + // In the gap — the event that used to go missing entirely. + serde_json::json!({ "created_at": "2026-06-13T14:53:00Z", "user": { "login": "mara" } }), + // The event being rendered right now — excluded (not "earlier"). + serde_json::json!({ "created_at": "2026-06-13T17:01:00Z", "user": { "login": "mara" } }), + ]; + assert_eq!( + count_unseen_earlier_events(&events, since, current, "damocles"), + 1 + ); + } + + #[test] + fn count_unseen_earlier_events_excludes_own_events() { + // The agent's own reply/action in the gap isn't "unseen" — it did it. + let since = parse_rfc3339("2026-06-13T10:00:00Z").unwrap(); + let current = parse_rfc3339("2026-06-13T17:01:00Z").unwrap(); + let events = [ + serde_json::json!({ "created_at": "2026-06-13T14:53:00Z", "user": { "login": "damocles" } }), + ]; + assert_eq!( + count_unseen_earlier_events(&events, since, current, "damocles"), + 0 + ); + } + + #[test] + fn count_unseen_earlier_events_zero_when_nothing_in_the_gap() { + // The common case: `latest_comment_url` really was the only new + // thing, no marker should ever fire for it. + let since = parse_rfc3339("2026-06-13T10:00:00Z").unwrap(); + let current = parse_rfc3339("2026-06-13T17:01:00Z").unwrap(); + let events = [ + serde_json::json!({ "created_at": "2026-06-13T17:01:00Z", "user": { "login": "mara" } }), + ]; + assert_eq!( + count_unseen_earlier_events(&events, since, current, "damocles"), + 0 + ); + } + + #[test] + fn count_unseen_earlier_events_ignores_unparseable_entries() { + // An entry missing/garbling `created_at` degrades to "not in the + // gap" rather than panicking or poisoning the count. + let since = parse_rfc3339("2026-06-13T10:00:00Z").unwrap(); + let current = parse_rfc3339("2026-06-13T17:01:00Z").unwrap(); + let events = [ + serde_json::json!({ "user": { "login": "mara" } }), + serde_json::json!({ "created_at": "not-a-date", "user": { "login": "mara" } }), + ]; + assert_eq!( + count_unseen_earlier_events(&events, since, current, "damocles"), + 0 + ); + } + + /// Regression for the whole point of the widening (mara's "count + /// everything" ruling): a non-comment timeline entry (a label add, no `body` + /// field, just `type`/`created_at`/`user`) must count exactly like a + /// comment does — the function has no type filter at all. + #[test] + fn count_unseen_earlier_events_counts_non_comment_types() { + let since = parse_rfc3339("2026-06-13T10:00:00Z").unwrap(); + let current = parse_rfc3339("2026-06-13T17:01:00Z").unwrap(); + let events = [ + serde_json::json!({ + "type": "label", + "created_at": "2026-06-13T14:53:00Z", + "user": { "login": "mara" }, + }), + serde_json::json!({ + "type": "pull_push", + "created_at": "2026-06-13T15:10:00Z", + "user": { "login": "mara" }, + }), + ]; + assert_eq!( + count_unseen_earlier_events(&events, since, current, "damocles"), + 2 + ); + } }