diff --git a/docs/forge.md b/docs/forge.md index f3bf9d00..c086fc05 100644 --- a/docs/forge.md +++ b/docs/forge.md @@ -140,14 +140,13 @@ lookahead. ### Wrapper format -Five shapes, distinguished by the notification's classification: +Four shapes, distinguished by the notification's classification: | Trigger | Wrapper | | --- | --- | | Comment on issue / PR | `[comment on PR #N owner/repo] title\nurl: ...\n\nauthor: body\nassignee: ...\nreason: mention` | | Review submission | `[PR approved #N owner/repo] title\nurl: ...\n\nreviewer: body\nassignee: ...\nreason: review_requested` | | New issue / PR | `[new PR #N owner/repo] title\nurl: ...\n\n\nassignee: ...\nreason: subscribed` | -| Later activity (open, not creation) | `[activity on PR #N owner/repo] title\nurl: ...\n\n\nassignee: ...\nreason: subscribed` | | State change | `[PR merged #N owner/repo] title\nurl: ...\nassignee: ...\nreason: subscribed` | Review labels come from the Forgejo `state` field: `APPROVED` → @@ -156,22 +155,6 @@ Review labels come from the Forgejo `state` field: `APPROVED` → submitted yet — no peer-visible event). Unknown states fall back to the generic comment wrapper. -### "new" vs "activity on" - -A review submitted with **no body** carries no `latest_comment_url`, -so it misses the comment path and lands on the state-change path with -`state == "open"` — exactly like a freshly opened PR. Labeling that -`new PR` is misleading: agents dismiss it as a duplicate of the -original open notification and miss the review (#1637). So the `open` -state only earns the `new ` label when the notification's event -time (`updated_at`) is within `NEW_ITEM_TOLERANCE_SECS` (120s) of the -subject's `created_at`. Anything later is labeled `activity on ` -— neutral and non-misleading, since we can't cheaply say *what* the -activity was without an extra reviews fetch. Missing/unparseable -timestamps default to `new` (preserve prior behavior rather than mask a -genuine new item). Timestamps are parsed by a small dependency-free -RFC 3339 → epoch-seconds helper (`parse_rfc3339_secs`). - Number is extracted from `subject.html_url`'s last path segment (strips `#anchor` first); repo slug from `repository.full_name`. Both degrade gracefully when absent (number → blank, repo → blank) diff --git a/hive-ag3nt/src/forge_notify.rs b/hive-ag3nt/src/forge_notify.rs index 86e0753b..072c32cf 100644 --- a/hive-ag3nt/src/forge_notify.rs +++ b/hive-ag3nt/src/forge_notify.rs @@ -27,13 +27,6 @@ const TOKEN_RETRY_SECS: u64 = 30; /// Give up waiting for the token after this many retries (~10 minutes). /// Avoids an infinite wait on agents that genuinely have no forge account. const TOKEN_RETRY_MAX: u32 = 20; -/// How close (seconds) the notification's event time must be to a -/// subject's `created_at` for us to call it a genuine creation and emit -/// a `new ` label. Later activity that lands on the state-change -/// path because it carries no `latest_comment_url` (e.g. a bodiless -/// review submission) fires well outside this window, so we must not -/// claim it's "new" — see docs/forge.md, "new vs activity on". -const NEW_ITEM_TOLERANCE_SECS: i64 = 120; /// Spawn point: called once from `hive serve`. Returns immediately if the forge is not /// configured. Otherwise loops forever, polling every @@ -584,20 +577,10 @@ fn format_state_change_notification( is_pr, } = meta; let label = notif_type_label(notif_type); - // Only claim "new" when the notification actually fired at creation - // time. A review submitted with no body carries no - // `latest_comment_url`, so it lands here instead of on the comment - // path — and its event time is well after `created_at`. Labeling - // that "new PR" is misleading (see docs/forge.md, "new vs activity - // on"): agents dismiss it as a - // duplicate of the original open notification. When we can't confirm - // creation, fall back to a neutral "activity on" label. - let looks_new = notification_is_creation(notif, subject.as_ref()); let kind = match notif_state { "merged" => format!("{label} merged{num}{repo}"), "closed" => format!("{label} closed{num}{repo}"), - "open" | "" if looks_new => format!("new {label}{num}{repo}"), - "open" | "" => format!("activity on {label}{num}{repo}"), + "open" | "" => format!("new {label}{num}{repo}"), other => format!("{label}{num}{repo}: {other}"), }; @@ -641,86 +624,6 @@ fn format_state_change_notification( Some(out) } -/// Decide whether a state-change notification represents the subject's -/// *creation* (so a `new ` label is truthful) versus later -/// activity that merely lacked a `latest_comment_url`. Compares the -/// notification's event time (`updated_at`) against the subject's -/// `created_at`: within `NEW_ITEM_TOLERANCE_SECS` ⇒ creation. When -/// either timestamp is missing or unparseable we default to `true`, -/// preserving the prior "new" behavior rather than masking a genuine -/// new item behind the neutral fallback. See docs/forge.md, "new vs -/// activity on". -fn notification_is_creation( - notif: &serde_json::Value, - subject: Option<&serde_json::Value>, -) -> bool { - let created = subject - .and_then(|s| s["created_at"].as_str()) - .and_then(parse_rfc3339_secs); - let event = notif["updated_at"].as_str().and_then(parse_rfc3339_secs); - match (created, event) { - (Some(c), Some(e)) => (e - c).abs() <= NEW_ITEM_TOLERANCE_SECS, - _ => true, - } -} - -/// Minimal dependency-free RFC 3339 / ISO 8601 parser → Unix epoch -/// seconds. Forgejo emits timestamps like `2026-06-13T11:18:42+02:00` -/// or `...Z`, optionally with fractional seconds. We only need -/// second-granularity comparison, so the fractional part is skipped. -/// Returns `None` on any shape we don't recognise so callers can fall -/// back gracefully. -fn parse_rfc3339_secs(s: &str) -> Option { - if s.len() < 19 { - return None; - } - let year: i64 = s.get(0..4)?.parse().ok()?; - let month: i64 = s.get(5..7)?.parse().ok()?; - let day: i64 = s.get(8..10)?.parse().ok()?; - let hour: i64 = s.get(11..13)?.parse().ok()?; - let minute: i64 = s.get(14..16)?.parse().ok()?; - let second: i64 = s.get(17..19)?.parse().ok()?; - - let mut epoch = - days_from_civil(year, month, day) * 86_400 + hour * 3_600 + minute * 60 + second; - - // Remainder after seconds: optional `.fff` fraction, then a zone. - let mut rest = &s[19..]; - if let Some(frac) = rest.strip_prefix('.') { - let end = frac - .find(|c: char| !c.is_ascii_digit()) - .unwrap_or(frac.len()); - rest = &frac[end..]; - } - // Zone: `Z`/empty = UTC; otherwise `±HH:MM`. Subtract the offset to - // normalise to UTC epoch seconds. - if !(rest.is_empty() || rest.starts_with('Z')) { - let sign = rest.as_bytes()[0]; - let off = &rest[1..]; - let oh: i64 = off.get(0..2)?.parse().ok()?; - let om: i64 = off.get(3..5).unwrap_or("00").parse().ok()?; - let offset = oh * 3_600 + om * 60; - match sign { - b'+' => epoch -= offset, - b'-' => epoch += offset, - _ => return None, - } - } - Some(epoch) -} - -/// Days since the Unix epoch for a proleptic-Gregorian `y-m-d` -/// (Howard Hinnant's `days_from_civil`). Valid for all dates Forgejo -/// can emit. -fn days_from_civil(y: i64, m: i64, d: i64) -> i64 { - let y = if m <= 2 { y - 1 } else { y }; - let era = (if y >= 0 { y } else { y - 399 }) / 400; - let yoe = y - era * 400; - let doy = (153 * (if m > 2 { m - 3 } else { m + 9 }) + 2) / 5 + d - 1; - let doe = yoe * 365 + yoe / 4 - yoe / 100 + doy; - era * 146_097 + doe - 719_468 -} - #[allow( clippy::too_many_arguments, reason = "the notification poll's config + mutable subscription state, \ @@ -1058,40 +961,4 @@ mod tests { "\n\nmentions (truncated from body):\n > cc @damocles\n > @argus second mention", ); } - - #[test] - fn parse_rfc3339_secs_handles_offsets_and_z() { - // Same instant expressed three ways must parse equal. - let utc = parse_rfc3339_secs("2026-06-13T09:18:42Z").unwrap(); - let plus2 = parse_rfc3339_secs("2026-06-13T11:18:42+02:00").unwrap(); - let minus5 = parse_rfc3339_secs("2026-06-13T04:18:42-05:00").unwrap(); - assert_eq!(utc, plus2); - assert_eq!(utc, minus5); - // Fractional seconds are skipped (second granularity). - assert_eq!(parse_rfc3339_secs("2026-06-13T09:18:42.512Z").unwrap(), utc); - } - - #[test] - fn parse_rfc3339_secs_rejects_garbage() { - assert!(parse_rfc3339_secs("").is_none()); - assert!(parse_rfc3339_secs("not-a-date").is_none()); - assert!(parse_rfc3339_secs("2026-06-13").is_none()); - } - - #[test] - fn notification_is_creation_flags_fresh_and_later_activity() { - // Fresh PR: notification event time == created_at → "new". - let fresh = serde_json::json!({ "updated_at": "2026-06-13T11:18:42+02:00" }); - let subject = serde_json::json!({ "created_at": "2026-06-13T11:18:40+02:00" }); - assert!(notification_is_creation(&fresh, Some(&subject))); - - // Review hours later on the same PR → not a creation. - let later = serde_json::json!({ "updated_at": "2026-06-13T14:55:00+02:00" }); - assert!(!notification_is_creation(&later, Some(&subject))); - - // Missing timestamps → default to creation (preserve prior behavior). - let empty = serde_json::json!({}); - assert!(notification_is_creation(&empty, None)); - assert!(notification_is_creation(&empty, Some(&subject))); - } }