diff --git a/docs/forge.md b/docs/forge.md index c086fc05..f3bf9d00 100644 --- a/docs/forge.md +++ b/docs/forge.md @@ -140,13 +140,14 @@ lookahead. ### Wrapper format -Four shapes, distinguished by the notification's classification: +Five 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` → @@ -155,6 +156,22 @@ 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 072c32cf..e3bf3cb4 100644 --- a/hive-ag3nt/src/forge_notify.rs +++ b/hive-ag3nt/src/forge_notify.rs @@ -27,6 +27,13 @@ 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 #1637. +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 @@ -577,10 +584,19 @@ 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 (#1637): 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" | "" => format!("new {label}{num}{repo}"), + "open" | "" if looks_new => format!("new {label}{num}{repo}"), + "open" | "" => format!("activity on {label}{num}{repo}"), other => format!("{label}{num}{repo}: {other}"), }; @@ -624,6 +640,85 @@ 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 #1637. +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, \ @@ -961,4 +1056,40 @@ 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 (#1637). + 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))); + } }