diff --git a/hive-ag3nt/src/forge_notify.rs b/hive-ag3nt/src/forge_notify.rs index e0682d6..a1f9e7a 100644 --- a/hive-ag3nt/src/forge_notify.rs +++ b/hive-ag3nt/src/forge_notify.rs @@ -211,7 +211,6 @@ fn review_state_label(state: &str) -> Option<&str> { /// /// Number is extracted from `html_url` last path segment before any `#`. /// Repo slug (`owner/name`) is always included — agents may watch multiple repos. -#[allow(clippy::too_many_lines)] // multiple notification types handled in one place by design async fn format_notification( client: &reqwest::Client, token: &str, @@ -256,157 +255,186 @@ async fn format_notification( }; let is_pr = matches!(notif_type, "Pull Request" | "Pull"); - - // Build assignee + reviewer suffix appended to all notification kinds. - let meta_suffix = { - let assignees: Vec<&str> = subject - .as_ref() - .and_then(|s| s["assignees"].as_array()) - .map(|arr| arr.iter().filter_map(|a| a["login"].as_str()).collect()) - .unwrap_or_default(); - let assignee_line = if assignees.is_empty() { - "assignee: unassigned".to_owned() - } else { - format!("assignee: {}", assignees.join(", ")) - }; - // For PRs, include requested_reviewers when present. - let reviewer_line = if is_pr { - let reviewers: Vec<&str> = subject - .as_ref() - .and_then(|s| s["requested_reviewers"].as_array()) - .map(|arr| arr.iter().filter_map(|r| r["login"].as_str()).collect()) - .unwrap_or_default(); - if reviewers.is_empty() { - None - } else { - Some(format!("reviewer: {}", reviewers.join(", "))) - } - } else { - None - }; - match reviewer_line { - Some(r) => format!("\n{assignee_line}\n{r}"), - None => format!("\n{assignee_line}"), - } - }; + let meta_suffix = build_meta_suffix(subject.as_ref(), is_pr); // 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; + let meta = NotifMeta { title, notif_type, html_url, num, repo, meta_suffix, subject, is_pr }; if has_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(""); - - // 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(); - - // 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 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() { - write!(out, "\n\nreviewer: {author}").ok(); - } else { - write!(out, "\n\n{author}: {}", truncate(body_text, BODY_TRUNCATE)).ok(); - } - out.push_str(&meta_suffix); - 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(); - } - out.push_str(&meta_suffix); - Some(out) - } + format_comment_notification(client, token, &meta, comment_api_url, comment_html_url, own_login).await } else { - // Notification triggered by creation or state change of the subject. - // - // Classification uses notif["subject"]["state"] directly — Forgejo - // returns "open" / "closed" / "merged" here. We do NOT rely on - // fetching the PR/issue detail for `merged`: - // - `subject.url` points to the *issues* endpoint, which returns - // `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(""); + format_state_change_notification(notif, &meta, own_login) + } +} - // 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; - } +/// Shared notification metadata extracted from the raw Forgejo JSON. +struct NotifMeta<'a> { + title: &'a str, + notif_type: &'a str, + html_url: &'a str, + num: String, + repo: String, + meta_suffix: String, + /// Fetched subject detail (issue/PR JSON); used for review-request detection. + subject: Option, + is_pr: bool, +} - let label = notif_type_label(notif_type); - let kind = match notif_state { - "merged" => format!("{label} merged{num}{repo}"), - "closed" => format!("{label} closed{num}{repo}"), - "open" | "" => format!("new {label}{num}{repo}"), - other => format!("{label}{num}{repo}: {other}"), - }; +/// Build the `\nassignee: ...` (and optionally `\nreviewer: ...`) suffix +/// appended to all notification kinds. +fn build_meta_suffix(subject: Option<&serde_json::Value>, is_pr: bool) -> String { + let assignees: Vec<&str> = subject + .and_then(|s| s["assignees"].as_array()) + .map(|arr| arr.iter().filter_map(|a| a["login"].as_str()).collect()) + .unwrap_or_default(); + let assignee_line = if assignees.is_empty() { + "assignee: unassigned".to_owned() + } else { + format!("assignee: {}", assignees.join(", ")) + }; + // For PRs, include requested_reviewers when present. + let reviewer_line = if is_pr { + let reviewers: Vec<&str> = subject + .and_then(|s| s["requested_reviewers"].as_array()) + .map(|arr| arr.iter().filter_map(|r| r["login"].as_str()).collect()) + .unwrap_or_default(); + if reviewers.is_empty() { None } else { Some(format!("reviewer: {}", reviewers.join(", "))) } + } else { + None + }; + match reviewer_line { + Some(r) => format!("\n{assignee_line}\n{r}"), + None => format!("\n{assignee_line}"), + } +} - // Review-request detection (#253): Forgejo does not always set - // reason == "review_requested" (observed reason is null). Check - // requested_reviewers instead, which is reliable. If own_login is - // in the list, this is a review request -- override the kind. - // `subject` and `is_pr` are already fetched unconditionally above (#256). - let is_review_request = is_new - && is_pr - && !own_login.is_empty() - && subject - .as_ref() - .and_then(|s| s["requested_reviewers"].as_array()) - .map(|arr| arr.iter().any(|r| r["login"].as_str() == Some(own_login))) - .unwrap_or(false); +/// Format a notification triggered by a new comment or review submission. +async fn format_comment_notification( + client: &reqwest::Client, + token: &str, + meta: &NotifMeta<'_>, + comment_api_url: &str, + comment_html_url: &str, + own_login: &str, +) -> Option { + let payload = fetch_json(client, comment_api_url, token).await; - let kind = if is_review_request { - format!("review requested{num}{repo}") + let actor_login = payload + .as_ref() + .and_then(|c| c["user"]["login"].as_str()) + .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(); + + // 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() { meta.html_url } else { comment_html_url }; + let author = if actor_login.is_empty() { "?" } else { actor_login }; + let NotifMeta { title, notif_type, num, repo, meta_suffix, .. } = meta; + + 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() { + write!(out, "\n\nreviewer: {author}").ok(); } else { - kind - }; - - let mut out = format!("[{kind}] {title}\nurl: {html_url}"); - out.push_str(&meta_suffix); + write!(out, "\n\n{author}: {}", truncate(body_text, BODY_TRUNCATE)).ok(); + } + out.push_str(meta_suffix); + 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(); + } + out.push_str(meta_suffix); Some(out) } } +/// Format a notification triggered by creation or state change of the subject. +fn format_state_change_notification( + notif: &serde_json::Value, + meta: &NotifMeta<'_>, + own_login: &str, +) -> Option { + // Classification uses notif["subject"]["state"] directly — Forgejo + // returns "open" / "closed" / "merged" here. We do NOT rely on + // fetching the PR/issue detail for `merged`: + // - `subject.url` points to the *issues* endpoint, which returns + // `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 NotifMeta { title, notif_type, html_url, num, repo, meta_suffix, subject, is_pr } = meta; + let label = notif_type_label(notif_type); + let kind = match notif_state { + "merged" => format!("{label} merged{num}{repo}"), + "closed" => format!("{label} closed{num}{repo}"), + "open" | "" => format!("new {label}{num}{repo}"), + other => format!("{label}{num}{repo}: {other}"), + }; + + // Review-request detection (#253): Forgejo does not always set + // reason == "review_requested" (observed as null). Check + // requested_reviewers instead, which is reliable. If own_login is + // in the list, override the kind. + // subject and is_pr are already fetched unconditionally above (#256). + let is_review_request = is_new + && *is_pr + && !own_login.is_empty() + && subject + .as_ref() + .and_then(|s| s["requested_reviewers"].as_array()) + .map(|arr| arr.iter().any(|r| r["login"].as_str() == Some(own_login))) + .unwrap_or(false); + let kind = if is_review_request { + format!("review requested{num}{repo}") + } else { + kind + }; + + let mut out = format!("[{kind}] {title}\nurl: {html_url}"); + out.push_str(meta_suffix); + Some(out) +} + #[allow(clippy::too_many_arguments)] async fn poll_once( client: &reqwest::Client, diff --git a/hive-ag3nt/src/web_ui.rs b/hive-ag3nt/src/web_ui.rs index 79d0c5b..993d97f 100644 --- a/hive-ag3nt/src/web_ui.rs +++ b/hive-ag3nt/src/web_ui.rs @@ -335,8 +335,7 @@ async fn api_stats( // Pass the window span to the reminder-stats RPC so the broker // filters its counts to the same time range as the chart data. let window_secs = window.span_secs(); - #[allow(clippy::cast_sign_loss)] // window span is always a positive duration - let window_secs_u = window_secs.max(0) as u64; + let window_secs_u = u64::try_from(window_secs).unwrap_or(0); snapshot.reminder_stats = fetch_reminder_stats(&state.socket, state.flavor(), window_secs_u).await; axum::Json(snapshot) } diff --git a/hive-c0re/src/broker.rs b/hive-c0re/src/broker.rs index ea88934..aa414ea 100644 --- a/hive-c0re/src/broker.rs +++ b/hive-c0re/src/broker.rs @@ -609,13 +609,13 @@ impl Broker { /// in the last `since_secs` seconds (0 = all reminders). pub fn reminder_rollup_for(&self, agent: &str, since_secs: u64) -> Result { let conn = self.conn.lock().unwrap(); - #[allow(clippy::cast_possible_wrap)] // unix epoch secs fit in i64 until year 292B let cutoff_time = if since_secs > 0 { let now = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .ok() - .map_or(0, |d| d.as_secs() as i64); - now - since_secs as i64 + .and_then(|d| i64::try_from(d.as_secs()).ok()) + .unwrap_or(0); + now.saturating_sub(i64::try_from(since_secs).unwrap_or(i64::MAX)) } else { i64::MIN };