vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(#2495): surface [PR merged] on merge notifications instead of [comment on PR]

Browse source
This commit is contained in:
damocles 2026-07-15 21:35:19 +02:00
commit 6aa7c9613b
2 changed files with 127 additions and 17 deletions
Download patch file Download diff file Expand all files Collapse all files

26
docs/forge.md
View file

@ -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: <author>` in
place of the `<author>: <body>` 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`,

118
hive-agent/src/forge_notify.rs
View file

@ -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<serde_json::Value> {
@ -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(&notif.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<OffsetDateTime>,
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