fix(#1637): don't label later activity (bodiless reviews) as new PR

2026-06-13 11:59:30 +02:00 · 2026-06-13 11:59:30 +02:00 · e6edfea0a8
commit e6edfea0a8
parent f05031ebe3
2 changed files with 150 additions and 2 deletions
--- 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<body excerpt>\nassignee: ...\nreason: subscribed` |
+| Later activity (open, not creation) | `[activity on PR #N owner/repo] title\nurl: ...\n\n<body excerpt>\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 <kind>` 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 <kind>`
+— 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)
--- 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 <kind>` 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 <kind>` 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<i64> {
+    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)));
+    }
 }