forge_notify: filter self-notifications, detect PR reviews (closes #230, closes #231)

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
+/// Returns `None` when the notification is a self-echo (actor is `own_login`)
-/// author and body. Falls back gracefully when API calls fail.
+/// and should be silently discarded (and marked read by the caller).
 ///
-/// Format: `[kind #N repo/name] title\nurl: <url>\n\nauthor: body` (comments)
+/// Formats:
-///         `[kind #N repo/name] title\nurl: <url>\nassignee: <login|unassigned>` (new items)
+/// - 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<String> {
     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
+    // Determine whether this notification was triggered by a comment/review or
-    // creation/state-change of the subject itself.
+    // 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.
+        // Notification triggered by a new comment or review submission.
-        let comment = fetch_json(client, comment_api_url, token).await;
+        let payload = fetch_json(client, comment_api_url, token).await;
-        let author = comment
+
+        let actor_login = payload
             .as_ref()
             .and_then(|c| c["user"]["login"].as_str())
-            .unwrap_or("?");
+            .unwrap_or("");
-        let body = comment
+
+        // 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));
+        let author = if actor_login.is_empty() { "?" } else { actor_login };
-        if out.ends_with('\n') {
+
-            out.pop();
+        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 <n>` 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<String>,
+    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}");
+        mark_read(client, forge_url, token, id).await;
-        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");
-            }
-        }
 
         // 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");
+        }
+    }
+}