diff --git a/docs/forge.md b/docs/forge.md index 7d38a965..b657191b 100644 --- a/docs/forge.md +++ b/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 : ` 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`, diff --git a/hive-agent/src/forge_notify.rs b/hive-agent/src/forge_notify.rs index cc97b712..f4794bbf 100644 --- a/hive-agent/src/forge_notify.rs +++ b/hive-agent/src/forge_notify.rs @@ -488,7 +488,33 @@ async fn format_notification( ) .await } else { - format_state_change_notification(notif.thread.updated_at, ¬if.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, + ¬if.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, ) -> Option { // 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 { + 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); } }