From 6aa7c9613b99861561d2877c2c8900d273fe3951 Mon Sep 17 00:00:00 2001 From: damocles Date: Wed, 15 Jul 2026 21:35:19 +0200 Subject: [PATCH] fix(#2495): surface [PR merged] on merge notifications instead of [comment on PR] --- docs/forge.md | 26 +++++++- hive-agent/src/forge_notify.rs | 118 +++++++++++++++++++++++++++++---- 2 files changed, 127 insertions(+), 17 deletions(-) diff --git a/docs/forge.md b/docs/forge.md index 83a2481c..7d38a965 100644 --- a/docs/forge.md +++ b/docs/forge.md @@ -178,9 +178,12 @@ silently (mark-read without delivery): `own_login`. Only _creations_ are dropped; a later state change on the agent's own subject is driven by someone else and still surfaces. -`own_login` is fetched once at startup via `GET /api/v1/user`. On -fetch failure the filter degrades open (no filtering) rather than -crashing the task — a noisy inbox beats a silently-stuck poller. +`own_login` is fetched at startup via `GET /api/v1/user`. On fetch +failure the filter degrades open (no filtering) rather than crashing +the task — a noisy inbox beats a silently-stuck poller — but the fetch +is **re-attempted on each poll tick** until it succeeds, so a boot-time +failure (the forge not yet reachable) self-heals instead of leaving +self-echo filtering off for the whole process lifetime. ### Body excerpt + truncation + heading escape @@ -236,6 +239,23 @@ A review submitted with **no body** renders `reviewed by: ` in place of the `: ` line — deliberately worded to not collide with the meta-suffix `reviewer:` line (requested reviewers, below). +### Merge/close vs a later comment + +A notification carrying a `latest_comment_url` normally takes the comment +path. But a merged/closed subject **keeps** its `latest_comment_url` set, +so a just-merged PR that had any prior discussion would route to the +comment path and render `[comment on PR]` (with a stale pre-merge comment +body) instead of `[PR merged]` — the agent never learns its PR merged +(#2495). So when the notification IS the merge/close transition — its +event time (`updated_at`) is within `NEW_ITEM_TOLERANCE_SECS` of the +subject's `closed_at` (set for both `merged` and `closed`) — the +state-change path wins even with a comment url present +(`state_change_is_current`). A genuine **later** comment on an +already-closed subject bumps `updated_at` well past `closed_at`, so it +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. + ### "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 47e5272c..cc97b712 100644 --- a/hive-agent/src/forge_notify.rs +++ b/hive-agent/src/forge_notify.rs @@ -138,20 +138,17 @@ pub async fn run(socket: PathBuf) { } }; - // Fetch own login once for self-notification filtering. Falls back - // to empty string on failure — no filtering (safe degradation; see - // `docs/forge.md::Self-notification filtering`). - let own_login = tokio::time::timeout( - Duration::from_secs(HTTP_TIMEOUT_SECS), - forge.user_get_current().send(), - ) - .await - .ok() - .and_then(Result::ok) - .and_then(|u| u.login) - .unwrap_or_default(); + // Fetch own login for self-notification filtering. Falls back to + // empty string on failure — no filtering (safe degradation; see + // `docs/forge.md::Self-notification filtering`). A boot-time failure + // (e.g. the forge not yet reachable) is re-attempted on each poll tick + // below rather than leaving filtering off for the whole process. + let mut own_login = resolve_own_login(&forge).await; if own_login.is_empty() { - warn!("forge_notify: could not resolve own login — self-notification filtering disabled"); + warn!( + "forge_notify: could not resolve own login yet — self-notification \ + filtering disabled until it resolves on a later poll" + ); } else { debug!(%own_login, "forge_notify: own login resolved"); } @@ -192,10 +189,35 @@ pub async fn run(socket: PathBuf) { loop { interval.tick().await; + // If own-login didn't resolve at boot (forge unreachable then), + // retry before this poll so self-echo filtering self-heals instead + // of staying off for the whole process lifetime. + if own_login.is_empty() { + own_login = resolve_own_login(&forge).await; + if !own_login.is_empty() { + debug!(%own_login, "forge_notify: own login resolved on retry"); + } + } poll_once(&forge, &client, &token, &socket, &mut delivered, &own_login).await; } } +/// Fetch the agent's own forge login (`GET /api/v1/user`) for +/// self-notification filtering. Returns the empty string on any failure +/// (timeout, HTTP error, missing field); the caller treats empty as +/// "filtering disabled" and retries on the next poll tick. +async fn resolve_own_login(forge: &Forgejo) -> String { + tokio::time::timeout( + Duration::from_secs(HTTP_TIMEOUT_SECS), + forge.user_get_current().send(), + ) + .await + .ok() + .and_then(Result::ok) + .and_then(|u| u.login) + .unwrap_or_default() +} + /// Fetch a JSON value from a URL using the agent's forge token. Returns /// `None` on any HTTP or parse error (best-effort enrichment). async fn fetch_json(client: &reqwest::Client, url: &str, token: &str) -> Option { @@ -447,7 +469,15 @@ async fn format_notification( subject, is_pr, }; - if has_comment { + // A merged/closed subject keeps its `latest_comment_url` set, so a + // just-merged PR that had any discussion would otherwise route to the + // comment path and render as `[comment on PR]` instead of `[PR merged]`. + // When this notification IS the merge/close transition, prefer + // the state-change path even with a comment url present; a genuine later + // comment stays on the comment path (see `state_change_is_current`). + 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 { format_comment_notification( client, token, @@ -716,6 +746,36 @@ fn notification_is_creation( } } +/// Whether this notification represents the subject's *own* merge/close +/// transition, as opposed to a later comment on an already-merged/closed +/// subject. A merged/closed PR keeps its `latest_comment_url` set, so +/// without this a just-merged PR would route to the comment path and +/// render as `[comment on PR]` instead of `[PR merged]`. We treat +/// the transition as current when the notification's event time +/// (`updated_at`) is within `NEW_ITEM_TOLERANCE_SECS` of the subject's +/// `closed_at` (set for both `merged` and `closed`). A genuine later +/// comment bumps `updated_at` well past `closed_at`, so it stays on the +/// comment path (and keeps its comment body). Missing/unparseable +/// timestamps default to `true` so a merge is never silently hidden +/// behind a stale comment — mirrors `notification_is_creation`'s +/// preserve-the-signal fallback. +fn state_change_is_current( + state: &str, + event: Option, + subject: Option<&serde_json::Value>, +) -> bool { + if !matches!(state, "merged" | "closed") { + return false; + } + let closed = subject + .and_then(|s| s["closed_at"].as_str()) + .and_then(parse_rfc3339); + match (closed, event) { + (Some(c), Some(e)) => (e - c).whole_seconds().abs() <= NEW_ITEM_TOLERANCE_SECS, + _ => true, + } +} + /// 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 @@ -1220,6 +1280,36 @@ mod tests { )); } + #[test] + fn state_change_is_current_distinguishes_merge_from_later_comment() { + // A merged/closed PR whose notification fired ~when it was closed → + // the transition itself → prefer the state-change ([PR merged]) path + // even though a merged PR keeps its latest_comment_url set. + let merged = serde_json::json!({ "closed_at": "2026-06-13T11:18:40+02:00" }); + let at_merge = parse_rfc3339("2026-06-13T11:18:42+02:00"); + assert!(state_change_is_current("merged", at_merge, Some(&merged))); + assert!(state_change_is_current("closed", at_merge, Some(&merged))); + + // A comment hours after the merge → not the transition → stays on the + // comment path so the comment body survives. + let later = parse_rfc3339("2026-06-13T14:55:00+02:00"); + assert!(!state_change_is_current("merged", later, Some(&merged))); + + // Non-terminal states never override the comment path. + assert!(!state_change_is_current("open", at_merge, Some(&merged))); + assert!(!state_change_is_current("", at_merge, Some(&merged))); + + // Missing/unparseable timestamps → default true so a merge is never + // hidden behind a stale comment. + assert!(state_change_is_current("merged", None, Some(&merged))); + assert!(state_change_is_current( + "merged", + at_merge, + Some(&serde_json::json!({})) + )); + assert!(state_change_is_current("merged", None, None)); + } + #[test] fn parse_notification_tolerates_merged_state() { // forgejo-api's `StateType` has no "merged" variant; the raw