forge_notify: append a comment racing the merge instead of dropping it (#2507)

docs/forge.md

@@ -256,6 +256,25 @@ stays on the comment path and keeps its comment body. Missing/unparseable
 timestamps default to the state-change path, so a merge is never silently
 hidden behind a stale comment.
 
+#### Merge racing a comment
+
+The one gap the timestamp cut leaves: a genuine comment posted **within
+`NEW_ITEM_TOLERANCE_SECS` of the merge** bumps `updated_at` close enough
+to `closed_at` that `state_change_is_current` returns `true` — so it takes
+the state-change path and its body would be dropped. Best of both worlds:
+on the merge/close path we fetch the `latest_comment_url` comment and, when
+its `created_at` is strictly **after** the subject's `closed_at`
+(`comment_is_after_close`) — i.e. it raced the merge rather than being the
+pre-merge last comment the subject keeps — append it as a
+`comment by <author>: <excerpt>` block before the meta suffix
+(`fresh_post_close_comment_tail`). So the wake carries **both** `[PR merged]`
+and the racing comment. The kept pre-merge comment (created before
+`closed_at`) is left off, a self-authored racing comment is dropped (don't
+echo the agent's own write), and a missing/unparseable `created_at`/
+`closed_at` appends nothing (conservative — only surface a comment we can
+positively place after the close). Cost: one extra comment fetch on
+merge/close notifications, acceptable given how rare they are.
+
 ### "new" vs "activity on"
 
 A review submitted with **no body** carries no `latest_comment_url`,

hive-agent/src/forge_notify.rs

@@ -488,7 +488,33 @@ async fn format_notification(
         )
         .await
     } else {
-        format_state_change_notification(notif.thread.updated_at, &notif.state, &meta, own_login)
+        // 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/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,
+                token,
+                comment_api_url,
+                meta.subject.as_ref(),
+                own_login,
+            )
+            .await
+        } else {
+            None
+        };
+        format_state_change_notification(
+            notif.thread.updated_at,
+            &notif.state,
+            &meta,
+            own_login,
+            comment_tail,
+        )
     }
 }
 
@@ -633,6 +659,7 @@ fn format_state_change_notification(
     notif_state: &str,
     meta: &NotifMeta<'_>,
     own_login: &str,
+    comment_tail: Option<String>,
 ) -> Option<String> {
     // Classification uses the raw `subject.state` string extracted in
     // `parse_notification` — Forgejo returns "open" / "closed" / "merged"
@@ -720,6 +747,11 @@ fn format_state_change_notification(
         .unwrap_or_default();
 
     let mut out = format!("[{kind}] {title}\nurl: {html_url}{body_block}");
+    // Append a comment that raced the merge/close (best of both worlds), when
+    // the caller found one genuinely newer than `closed_at`.
+    if let Some(tail) = comment_tail {
+        out.push_str(&tail);
+    }
     out.push_str(meta_suffix);
     Some(out)
 }
@@ -776,6 +808,60 @@ fn state_change_is_current(
     }
 }
 
+/// True when the (already-fetched) comment payload was created strictly
+/// after the subject's `closed_at` — i.e. a comment racing the merge/close,
+/// not the pre-merge last comment a merged/closed subject keeps on
+/// `latest_comment_url`. Unlike `state_change_is_current`, a missing or
+/// unparseable timestamp defaults to `false`: we only append a comment we
+/// can positively place after the close, so an unplaceable one is never
+/// bolted onto a merge message where it might be stale.
+fn comment_is_after_close(
+    comment: &serde_json::Value,
+    subject: Option<&serde_json::Value>,
+) -> bool {
+    let closed = subject
+        .and_then(|s| s["closed_at"].as_str())
+        .and_then(parse_rfc3339);
+    let created = comment["created_at"].as_str().and_then(parse_rfc3339);
+    match (created, closed) {
+        (Some(cr), Some(cl)) => cr > cl,
+        _ => false,
+    }
+}
+
+/// Render the trailing comment block to append to a merge/close notification
+/// when a comment raced the merge (posted after `closed_at`). Fetches the
+/// comment via `latest_comment_url` and returns `None` when it predates the
+/// close (the ordinary kept last comment), is self-authored (don't echo the
+/// agent's own write back at it), is empty/bodiless, or can't be fetched.
+/// This is the one extra fetch the merge/close path pays for the best of
+/// both worlds — cheap given how rare merge notifications are.
+async fn fresh_post_close_comment_tail(
+    client: &reqwest::Client,
+    token: &str,
+    comment_api_url: &str,
+    subject: Option<&serde_json::Value>,
+    own_login: &str,
+) -> Option<String> {
+    let payload = fetch_json(client, comment_api_url, token).await?;
+    if !comment_is_after_close(&payload, subject) {
+        return None;
+    }
+    let author = payload["user"]["login"].as_str().unwrap_or("");
+    // Don't surface the agent's own racing comment back to it — mirrors the
+    // self-authored filter on the comment/review path.
+    if !own_login.is_empty() && author == own_login {
+        return None;
+    }
+    let body = payload["body"].as_str().unwrap_or("").trim();
+    if body.is_empty() {
+        return None;
+    }
+    let author = if author.is_empty() { "?" } else { author };
+    let (excerpt, mentions) = render_body_excerpt(body);
+    Some(format!("\n\ncomment by {author}: {excerpt}{mentions}"))
+}
+
 /// 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
@@ -1378,13 +1464,13 @@ 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").is_none());
+        assert!(format_state_change_notification(None, "open", &meta, "damocles", 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").is_some());
+        assert!(format_state_change_notification(None, "open", &meta, "damocles", None).is_some());
     }
 
     #[test]
@@ -1397,6 +1483,58 @@ mod tests {
             "created_at": "2020-01-01T00:00:00Z",
         }));
         let event = parse_rfc3339("2026-06-22T16:00:00Z");
-        assert!(format_state_change_notification(event, "closed", &meta, "damocles").is_some());
+        assert!(
+            format_state_change_notification(event, "closed", &meta, "damocles", None).is_some()
+        );
+    }
+
+    #[test]
+    fn comment_is_after_close_distinguishes_racing_from_kept_comment() {
+        let subject = serde_json::json!({ "closed_at": "2026-06-13T11:18:40+02:00" });
+
+        // Comment posted a minute after the merge → a comment racing the
+        // merge → surface it.
+        let racing = serde_json::json!({ "created_at": "2026-06-13T11:19:40+02:00" });
+        assert!(comment_is_after_close(&racing, Some(&subject)));
+
+        // The pre-merge last comment kept on `latest_comment_url` predates
+        // the close → must NOT be appended.
+        let kept = serde_json::json!({ "created_at": "2026-06-13T10:00:00+02:00" });
+        assert!(!comment_is_after_close(&kept, Some(&subject)));
+
+        // Missing/unparseable timestamps → false (don't append a comment we
+        // can't place after the close), opposite of the state-change default.
+        assert!(!comment_is_after_close(&racing, None));
+        assert!(!comment_is_after_close(
+            &serde_json::json!({}),
+            Some(&subject)
+        ));
+        assert!(!comment_is_after_close(
+            &racing,
+            Some(&serde_json::json!({}))
+        ));
+    }
+
+    #[test]
+    fn format_state_change_appends_comment_tail() {
+        // The racing-comment tail lands between the body block and the meta
+        // suffix, so the assignee line stays last.
+        let meta = state_change_meta(serde_json::json!({
+            "user": { "login": "someone-else" },
+            "body": "the PR description",
+        }));
+        let out = format_state_change_notification(
+            None,
+            "merged",
+            &meta,
+            "damocles",
+            Some("\n\ncomment by argus: nice, merging".to_owned()),
+        )
+        .expect("merge notification must render");
+        assert!(out.contains("comment by argus: nice, merging"));
+        // Tail precedes the meta suffix.
+        let tail_at = out.find("comment by argus").unwrap();
+        let assignee_at = out.find("assignee:").unwrap();
+        assert!(tail_at < assignee_at);
     }
 }