diff --git a/hive-ag3nt/src/forge_notify.rs b/hive-ag3nt/src/forge_notify.rs index 165164d..6c179df 100644 --- a/hive-ag3nt/src/forge_notify.rs +++ b/hive-ag3nt/src/forge_notify.rs @@ -16,6 +16,20 @@ //! After successfully delivering a notification it is marked read via //! `PATCH /notifications/threads/{id}` so it does not re-fire. If delivery //! fails the thread is left unread so it resurfaces next tick. +//! +//! Self-notification filtering (closes #230): +//! - New issues/PRs created by this agent (`reason == "author"` + `state == open`) +//! are silently marked read — the agent already knows it opened them. +//! - Comment notifications where the comment author matches this agent's own +//! forge login are silently marked read. +//! Own login is fetched once at startup via `GET /user` and cached for the +//! lifetime of the polling loop. +//! +//! PR review formatting (closes #231): +//! - When `latest_comment_url` points to a review (the fetched JSON has a +//! `state` field like `APPROVED` / `REQUEST_CHANGES` / `COMMENT`), the +//! notification is formatted as `[PR approved #N repo]` instead of the +//! generic `[comment on PR #N repo]` so agents can action it immediately. use std::collections::HashSet; use std::path::{Path, PathBuf}; @@ -73,6 +87,21 @@ pub async fn run(socket: PathBuf, is_manager: bool) { } }; + // Fetch own login once for self-notification filtering (closes #230). + // Falls back to empty string on failure — no filtering (safe degradation). + let own_login = { + let url = format!("{forge_url}/api/v1/user"); + fetch_json(&client, &url, &token) + .await + .and_then(|v| v["login"].as_str().map(|s| s.to_owned())) + .unwrap_or_default() + }; + if own_login.is_empty() { + warn!("forge_notify: could not resolve own login — self-notification filtering disabled"); + } else { + debug!(%own_login, "forge_notify: own login resolved"); + } + info!(forge_url = %forge_url, "forge_notify: polling started"); let mut interval = tokio::time::interval(Duration::from_secs(POLL_INTERVAL_SECS)); @@ -101,6 +130,7 @@ pub async fn run(socket: PathBuf, is_manager: bool) { is_manager, keep_subscriptions, &mut unsubbed_repos, + &own_login, ) .await; } @@ -151,20 +181,37 @@ fn truncate(s: &str, max: usize) -> String { format!("{}…", &s[..end]) } +/// Map a Forgejo review state to a readable action label. +/// Returns `None` for non-review states (regular comments have no `state` field; +/// `PENDING` means the review was saved but not submitted yet). +/// Forgejo review states: "APPROVED", "REQUEST_CHANGES", "COMMENT", "PENDING". +fn review_state_label(state: &str) -> Option<&str> { + match state { + "APPROVED" => Some("approved"), + "REQUEST_CHANGES" => Some("changes requested"), + "COMMENT" => Some("review comment"), + _ => None, + } +} + /// Build a human-readable wake message for one Forgejo notification. -/// Fetches the subject and (if present) the latest comment to include -/// author and body. Falls back gracefully when API calls fail. +/// Returns `None` when the notification is a self-echo (actor is `own_login`) +/// and should be silently discarded (and marked read by the caller). /// -/// Format: `[kind #N repo/name] title\nurl: \n\nauthor: body` (comments) -/// `[kind #N repo/name] title\nurl: \nassignee: ` (new items) +/// Formats: +/// - Comment: `[comment on PR #N repo] title\nurl: ...\n\nauthor: body` +/// - Review: `[PR approved #N repo] title\nurl: ...\n\nreviewer: body` +/// - New item: `[new issue #N repo] title\nurl: ...\nassignee: user` +/// - State: `[PR merged #N repo] title\nurl: ...` /// -/// Number is extracted from `html_url` (last path segment before any `#`). +/// Number is extracted from `html_url` last path segment before any `#`. /// Repo slug (`owner/name`) is always included — agents may watch multiple repos. async fn format_notification( client: &reqwest::Client, token: &str, notif: &serde_json::Value, -) -> String { + own_login: &str, +) -> Option { let title = notif["subject"]["title"].as_str().unwrap_or("?"); let notif_type = notif["subject"]["type"].as_str().unwrap_or("?"); let html_url = notif["subject"]["html_url"] @@ -194,30 +241,62 @@ async fn format_notification( .as_str() .unwrap_or(""); - // Determine whether this notification was triggered by a comment or by - // creation/state-change of the subject itself. + // Determine whether this notification was triggered by a comment/review or + // by creation/state-change of the subject itself. let has_comment = !comment_api_url.is_empty() && comment_api_url != subject_api_url; if has_comment { - // Notification triggered by a new comment. - let comment = fetch_json(client, comment_api_url, token).await; - let author = comment + // Notification triggered by a new comment or review submission. + let payload = fetch_json(client, comment_api_url, token).await; + + let actor_login = payload .as_ref() .and_then(|c| c["user"]["login"].as_str()) - .unwrap_or("?"); - let body = comment + .unwrap_or(""); + + // Self-notification filter (#230): skip if we authored the comment/review. + if !own_login.is_empty() && actor_login == own_login { + debug!(%own_login, "forge_notify: skipping self-authored comment/review"); + return None; + } + + let body_text = payload .as_ref() .and_then(|c| c["body"].as_str()) .unwrap_or("") .trim(); - let kind = format!("comment on {}{num}{repo}", notif_type_label(notif_type)); + // PR review detection (#231): Forgejo review objects carry a `state` field + // with values like "APPROVED" / "REQUEST_CHANGES" / "COMMENT". Regular + // issue/PR comments have no such field. Format reviews distinctly so the + // agent knows the review outcome immediately without reading the body. + let review_state = payload + .as_ref() + .and_then(|c| c["state"].as_str()) + .and_then(review_state_label); + let url = if comment_html_url.is_empty() { html_url } else { comment_html_url }; - let mut out = format!("[{kind}] {title}\nurl: {url}\n\n{author}: {}", truncate(body, BODY_TRUNCATE)); - if out.ends_with('\n') { - out.pop(); + let author = if actor_login.is_empty() { "?" } else { actor_login }; + + if let Some(review_label) = review_state { + // Review submission on a PR. + let kind = format!("PR {review_label}{num}{repo}"); + let mut out = format!("[{kind}] {title}\nurl: {url}"); + if !body_text.is_empty() { + out.push_str(&format!("\n\n{author}: {}", truncate(body_text, BODY_TRUNCATE))); + } else { + out.push_str(&format!("\n\nreviewer: {author}")); + } + Some(out) + } else { + // Regular comment. + let kind = format!("comment on {}{num}{repo}", notif_type_label(notif_type)); + let mut out = format!("[{kind}] {title}\nurl: {url}\n\n{author}: {}", truncate(body_text, BODY_TRUNCATE)); + if out.ends_with('\n') { + out.pop(); + } + Some(out) } - out } else { // Notification triggered by creation or state change of the subject. // @@ -228,6 +307,17 @@ async fn format_notification( // `pull_request.merged`, not top-level `merged`. // - Forgejo API type is "Pull" / "Issue", never "Pull Request". let notif_state = notif["subject"]["state"].as_str().unwrap_or(""); + let reason = notif["reason"].as_str().unwrap_or(""); + + // Self-notification filter (#230): skip new items we authored ourselves. + // `reason == "author"` combined with open state means we just opened the + // issue/PR. We do NOT filter merged/closed state changes — those are + // triggered by someone else and we want them. + let is_new = notif_state == "open" || notif_state.is_empty(); + if is_new && reason == "author" && !own_login.is_empty() { + debug!(%own_login, "forge_notify: skipping self-authored new item"); + return None; + } let label = notif_type_label(notif_type); let kind = match notif_state { @@ -242,7 +332,6 @@ async fn format_notification( // For new (open) items: fetch assignee(s). Skip body — agents can // run `hive-forge view ` for full content. One extra API call // only on creation events, not state changes. - let is_new = notif_state == "open" || notif_state.is_empty(); if is_new { let subject = fetch_json(client, subject_api_url, token).await; let assignees: Vec<&str> = subject @@ -260,10 +349,11 @@ async fn format_notification( out.push_str(&format!("\nassignee: {}", assignees.join(", "))); } } - out + Some(out) } } +#[allow(clippy::too_many_arguments)] async fn poll_once( client: &reqwest::Client, forge_url: &str, @@ -272,6 +362,7 @@ async fn poll_once( is_manager: bool, keep_subscriptions: bool, unsubbed_repos: &mut HashSet, + own_login: &str, ) { let url = format!("{forge_url}/api/v1/notifications?all=false&limit=50"); let resp = match client @@ -312,7 +403,16 @@ async fn poll_once( None => continue, }; - let body = format_notification(client, token, notif).await; + let body_opt = format_notification(client, token, notif, own_login).await; + + // None means self-echo — mark read silently, no delivery. + let body = match body_opt { + Some(b) => b, + None => { + mark_read(client, forge_url, token, id).await; + continue; + } + }; let delivered = if is_manager { let req = hive_sh4re::ManagerRequest::Wake { @@ -343,23 +443,7 @@ async fn poll_once( // Mark as read only after successful delivery so a failed-delivery // notification resurfaces on the next poll tick. - let mark_url = format!("{forge_url}/api/v1/notifications/threads/{id}"); - match client - .patch(&mark_url) - .header("Authorization", format!("token {token}")) - .send() - .await - { - Err(e) => { - warn!(%id, error = ?e, "forge_notify: mark-read request failed — notification will resurface"); - } - Ok(r) if !r.status().is_success() => { - warn!(%id, status = %r.status(), "forge_notify: mark-read returned non-2xx — notification will resurface"); - } - Ok(_) => { - debug!(%id, "forge_notify: marked read"); - } - } + mark_read(client, forge_url, token, id).await; // Auto-unsubscribe from broad repo watches when the notification // reason is "subscribed" (agent watching the whole repo). Skipped @@ -392,3 +476,27 @@ async fn poll_once( } } } + +/// Mark a notification thread as read. Best-effort — logs on failure but +/// does not abort the poll loop. A notification left unread will resurface +/// on the next poll tick (desirable for delivery failures; for self-echo +/// silencing we call this without prior delivery). +async fn mark_read(client: &reqwest::Client, forge_url: &str, token: &str, id: u64) { + let mark_url = format!("{forge_url}/api/v1/notifications/threads/{id}"); + match client + .patch(&mark_url) + .header("Authorization", format!("token {token}")) + .send() + .await + { + Err(e) => { + warn!(%id, error = ?e, "forge_notify: mark-read request failed — notification will resurface"); + } + Ok(r) if !r.status().is_success() => { + warn!(%id, status = %r.status(), "forge_notify: mark-read returned non-2xx — notification will resurface"); + } + Ok(_) => { + debug!(%id, "forge_notify: marked read"); + } + } +}