fix(#2327): handle dismissed reviews like stale (superseded helper)

hive-forge/src/verbs/mod.rs

@@ -121,18 +121,43 @@ pub(crate) fn pct_encode(s: &str) -> String {
     out
 }
 
-/// Latest non-comment review verdict per reviewer on a PR, as
-/// `(login, state)`. Reviews come oldest-first, so a later verdict from
-/// the same user supersedes an earlier one; `COMMENT` / `PENDING`
-/// reviews carry no verdict and are skipped. Shared by `pr-status` (for
-/// its health view + readiness verdict) and `pr-merge` (for its
-/// pre-merge changes-requested gate) so the verdict semantics stay in
-/// one place.
-pub(crate) fn latest_reviews(
-    client: &Client,
-    repo: &str,
-    pr: u64,
-) -> Result<Vec<(String, String)>> {
+/// One reviewer's latest verdict on a PR, plus forgejo's `stale` /
+/// `dismissed` bits.
+///
+/// `stale` is set by forgejo when the PR head commit changed after this
+/// review was submitted (branch protection then wants a fresh review);
+/// `dismissed` is set when the review was explicitly dismissed. Either way
+/// the review no longer applies to the current head even though its `state`
+/// string still reads `APPROVED` / `REQUEST_CHANGES` — so surfacing them
+/// stops the CLI from reporting a no-longer-valid review as still-good, and
+/// [`ReviewInfo::superseded`] rolls both into one "doesn't count" check.
+pub(crate) struct ReviewInfo {
+    pub login: String,
+    pub state: String,
+    pub stale: bool,
+    pub dismissed: bool,
+}
+
+impl ReviewInfo {
+    /// True when the review no longer applies to the current head — stale
+    /// (head moved) or dismissed. Such a verdict neither blocks a merge nor
+    /// counts as a fresh approval.
+    pub(crate) fn superseded(&self) -> bool {
+        self.stale || self.dismissed
+    }
+}
+
+/// Latest non-comment review per reviewer on a PR. Reviews come
+/// oldest-first, so a later verdict from the same user supersedes an
+/// earlier one; `COMMENT` / `PENDING` reviews carry no verdict and are
+/// skipped. Shared by `pr-status` (health view + readiness verdict) and
+/// `pr-merge` (pre-merge changes-requested gate) so the verdict semantics
+/// stay in one place.
+///
+/// # Errors
+///
+/// Propagates the forge API errors from listing the PR's reviews.
+pub(crate) fn latest_reviews(client: &Client, repo: &str, pr: u64) -> Result<Vec<ReviewInfo>> {
     let (owner, name) = crate::client::split_repo(repo)?;
     let pr = index(pr)?;
     // Paginate (50/page, 10-page runaway cap — same ceiling the raw
@@ -151,7 +176,7 @@ pub(crate) fn latest_reviews(
             break;
         }
     }
-    let mut latest: Vec<(String, String)> = Vec::new();
+    let mut latest: Vec<ReviewInfo> = Vec::new();
     for r in &reviews {
         let Some(login) = r.user.as_ref().and_then(|u| u.login.as_deref()) else {
             continue;
@@ -160,10 +185,19 @@ pub(crate) fn latest_reviews(
         if st == "COMMENT" || st == "PENDING" || st.is_empty() {
             continue;
         }
-        if let Some(slot) = latest.iter_mut().find(|(l, _)| l == login) {
-            st.clone_into(&mut slot.1);
+        let stale = r.stale.unwrap_or(false);
+        let dismissed = r.dismissed.unwrap_or(false);
+        if let Some(slot) = latest.iter_mut().find(|info| info.login == login) {
+            st.clone_into(&mut slot.state);
+            slot.stale = stale;
+            slot.dismissed = dismissed;
         } else {
-            latest.push((login.to_owned(), st.to_owned()));
+            latest.push(ReviewInfo {
+                login: login.to_owned(),
+                state: st.to_owned(),
+                stale,
+                dismissed,
+            });
         }
     }
     Ok(latest)

hive-forge/src/verbs/pr_merge.rs

@@ -157,11 +157,13 @@ fn check_ready(client: &Client, repo: &str, number: u64, pull: &PullRequest) ->
 
     // Block only on reviewers whose *current* verdict requests changes
     // (latest-per-reviewer, so a later APPROVED clears an earlier
-    // REQUEST_CHANGES). Shares the verdict logic with `pr-status`.
+    // REQUEST_CHANGES). A superseded REQUEST_CHANGES — stale (older head) or
+    // dismissed — no longer applies, so it doesn't block. Shares the verdict
+    // logic with `pr-status`.
     let blockers: Vec<String> = super::latest_reviews(client, repo, number)?
         .into_iter()
-        .filter(|(_, st)| st == "REQUEST_CHANGES")
-        .map(|(login, _)| login)
+        .filter(|r| r.state == "REQUEST_CHANGES" && !r.superseded())
+        .map(|r| r.login)
         .collect();
     if !blockers.is_empty() {
         bail!(

hive-forge/src/verbs/pr_reviews.rs

@@ -135,6 +135,13 @@ fn list_reviews_json(client: &Client, number: u64, reviews: &[Value]) -> Result<
             json!({
                 "id": r.get("id"),
                 "state": r.get("state"),
+                // forgejo's staleness bits: `stale` = head moved since the
+                // review (branch protection wants a fresh one); `dismissed`
+                // = explicitly dismissed. A stale/dismissed APPROVED no
+                // longer satisfies the merge gate despite `state` reading
+                // APPROVED — surface them so callers don't over-trust it.
+                "stale": r.get("stale"),
+                "dismissed": r.get("dismissed"),
                 "user": r.get("user").and_then(|u| u.get("login")),
                 "body": r.get("body"),
                 "comments_count": r.get("comments_count"),
@@ -161,7 +168,18 @@ fn list_reviews_text(client: &Client, number: u64, reviews: &[Value]) {
             .unwrap_or("?");
         let state = r.get("state").and_then(Value::as_str).unwrap_or("?");
         let body = r.get("body").and_then(Value::as_str).unwrap_or("").trim();
-        println!("### review by {user} ({state})");
+        // Flag reviews forgejo considers no-longer-current so a stale
+        // APPROVED doesn't read as still-satisfying branch protection.
+        let is_stale = r.get("stale").and_then(Value::as_bool).unwrap_or(false);
+        let is_dismissed = r.get("dismissed").and_then(Value::as_bool).unwrap_or(false);
+        let flag = if is_stale {
+            " [stale — needs re-review]"
+        } else if is_dismissed {
+            " [dismissed]"
+        } else {
+            ""
+        };
+        println!("### review by {user} ({state}){flag}");
         if !body.is_empty() {
             println!("{body}");
         }

hive-forge/src/verbs/pr_status.rs

@@ -120,7 +120,10 @@ fn pr_status(client: &Client, repo: &str, pr: u64) -> Result<()> {
             "requested_reviewers": requested,
             "reviews": reviews
                 .iter()
-                .map(|(l, s)| serde_json::json!({"user": l, "state": s}))
+                .map(|r| serde_json::json!({
+                    "user": r.login, "state": r.state,
+                    "stale": r.stale, "dismissed": r.dismissed,
+                }))
                 .collect::<Vec<_>>(),
             "last_comment": last
                 .as_ref()
@@ -143,9 +146,12 @@ fn pr_status(client: &Client, repo: &str, pr: u64) -> Result<()> {
     }
 
     // Merge-readiness: CI green, mergeable, and nobody requesting changes.
+    // A *superseded* REQUEST_CHANGES — stale (branch moved since) or
+    // dismissed — no longer applies to the current head, so it doesn't hold
+    // up the verdict.
     let changes_requested = reviews
         .iter()
-        .any(|(_, verdict)| verdict == "REQUEST_CHANGES");
+        .any(|r| r.state == "REQUEST_CHANGES" && !r.superseded());
     let ready = ci_state == "success" && mergeable == Some(true) && !changes_requested;
     if ready {
         Ok(())
@@ -280,7 +286,7 @@ fn print_pr(
     ci_state: &str,
     ci_statuses: &[Value],
     requested: &[String],
-    reviews: &[(String, String)],
+    reviews: &[super::ReviewInfo],
     last_comment: Option<&(String, String)>,
 ) {
     println!("PR #{pr}: {title}");
@@ -310,13 +316,26 @@ fn print_pr(
     } else {
         let rendered: Vec<String> = reviews
             .iter()
-            .map(|(login, verdict)| {
-                let mark = match verdict.as_str() {
+            .map(|r| {
+                let mark = match r.state.as_str() {
+                    // A superseded approval (stale/dismissed) no longer
+                    // satisfies branch protection; flag it so the CLI doesn't
+                    // read as still-good.
+                    "APPROVED" if r.superseded() => "⚠️",
                     "APPROVED" => "✅",
                     "REQUEST_CHANGES" => "❌",
                     _ => "•",
                 };
-                format!("{mark} {login}: {verdict}")
+                let login = &r.login;
+                let state = &r.state;
+                let suffix = if r.stale {
+                    " (stale — needs re-review on current head)"
+                } else if r.dismissed {
+                    " (dismissed)"
+                } else {
+                    ""
+                };
+                format!("{mark} {login}: {state}{suffix}")
             })
             .collect();
         println!("  reviews:      {}", rendered.join(", "));