forge-notify: widen earlier-activity marker to the full timeline

hive-forge-notify/src/notify.rs

@@ -260,6 +260,7 @@ async fn format_notification<S: Source>(
     source: &S,
     notif: &PolledNotification,
     own_login: &str,
+    prev_updated_at: Option<&str>,
 ) -> Option<String> {
     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<S: Source>(
     let is_fresh_state_change =
         state_change_is_current(&notif.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<S: Source>(
         } 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,
             &notif.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<S: Source>(
     client: &reqwest::Client,
@@ -433,6 +470,7 @@ async fn format_comment_notification<S: Source>(
     meta: &NotifMeta<'_>,
     comment_api_url: &str,
     comment_html_url: &str,
+    enrichment: &CommentEnrichment<'_>,
     own_login: &str,
 ) -> Option<String> {
     let payload = fetch_json(client, comment_api_url, source).await;
@@ -504,11 +542,99 @@ async fn format_comment_notification<S: Source>(
         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<S: Source>(
+    client: &reqwest::Client,
+    source: &S,
+    subject_api_url: &str,
+    since: &str,
+    current: OffsetDateTime,
+    own_login: &str,
+) -> Option<usize> {
+    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<usize>) -> 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<String>,
+    earlier_updates: Option<usize>,
 ) -> Option<String> {
     // 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<S: Source, H: std::hash::BuildHasher>(
         // 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
+        );
+    }
 }